Skip to content

fix: harden cookie flags, sanitize Genie markdown output, fix remote tunnel #216

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pkosiec merged 3 commits into from
Mar 26, 2026
Merged

fix: harden cookie flags, sanitize Genie markdown output, fix remote tunnel #216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6ef9a96
fix: harden cookie flags and sanitize Genie markdown output
pkosiec Mar 25, 2026
da0912c
fix: copy remote tunnel HTML files to dist during build
pkosiec Mar 26, 2026
9d8ff7f
fix(ci): use npm install for PR template artifact step
pkosiec Mar 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ jobs:

- name: Install template dependencies
working-directory: pr-template
run: npm ci
run: npm install
Copy link
Copy Markdown
Member Author

@pkosiec pkosiec Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prepare-pr-template.ts rewrites dependencies to file: tarballs, which will never match the lockfile copied from template - so we need to revert this

Copy link
Copy Markdown
Collaborator

@MarioCadenas MarioCadenas Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, but I guess now we have fixed deps it should be fine

Copy link
Copy Markdown
Member Author

@pkosiec pkosiec Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No - see this failure: https://github.com/databricks/appkit/actions/runs/23590462691/job/68694129912
The prepare-pr-template.ts script rewrites deps from registry specifiers to file paths. The lockfile still references the original registry URLs, so npm ci will fail because the specifiers don't match - regardless of whether versions are pinned.

Copy link
Copy Markdown
Collaborator

@MarioCadenas MarioCadenas Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant keeping npm install sorry, I understand we can't keep npm ci in there


- name: Create zip artifact
working-directory: pr-template
Expand Down
2 changes: 1 addition & 1 deletion apps/dev-playground/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
"build": "npm run build:app",
"build:app": "tsdown --out-dir build server/index.ts && cd client && npm run build",
"build:server": "tsdown --out-dir build server/index.ts",
"install": "cd client && npm ci && cd ..",
"install": "cd client && npm install && cd ..",
Copy link
Copy Markdown
Member Author

@pkosiec pkosiec Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CC @MarioCadenas we need to revert that, otherwise the pnpm deploy:playground doesn't work. The dependencies need to be downloaded on runtime side overriding the local ones (from the npm registry proxy)

Copy link
Copy Markdown
Collaborator

@MarioCadenas MarioCadenas Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so? npm ci sould work anyway, what's the problem with that?

Copy link
Copy Markdown
Member Author

@pkosiec pkosiec Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, because the lock file doesn't match - when pnpm deploy:playground runs, the deployed package.json has rewritten dependencies (file: tarballs replaced with registry versions), which means the lockfile no longer matches

Copy link
Copy Markdown
Collaborator

@MarioCadenas MarioCadenas Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we running this one on ci at any moment? or just locally?

Copy link
Copy Markdown
Member Author

@pkosiec pkosiec Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, only locally

"preview": "vite preview",
"check": "tsc",
"clean": "rm -rf build && cd client && rm -rf dist",
Expand Down
1 change: 1 addition & 0 deletions packages/appkit-ui/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@
"class-variance-authority": "0.7.1",
"clsx": "2.1.1",
"cmdk": "1.1.1",
"dompurify": "3.3.3",
"echarts": "6.0.0",
"echarts-for-react": "3.0.5",
"embla-carousel-react": "8.6.0",
Expand Down
8 changes: 6 additions & 2 deletions packages/appkit-ui/src/react/genie/genie-chat-message.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import DOMPurify from "dompurify";
import { marked } from "marked";
import { useMemo } from "react";
import { cn } from "../lib/utils";
Expand All @@ -9,7 +10,7 @@ import type { GenieAttachmentResponse, GenieMessageItem } from "./types";
/**
* Using `marked` instead of `react-markdown` because `react-markdown` depends on
* `micromark-util-symbol` which has broken ESM exports with `rolldown-vite`.
* Content comes from our own Genie API so `dangerouslySetInnerHTML` is safe.
* Output is sanitized with DOMPurify before being passed to `dangerouslySetInnerHTML`.
*/
marked.setOptions({ breaks: true, gfm: true });

Expand Down Expand Up @@ -43,7 +44,10 @@ export function GenieChatMessage({
const isUser = message.role === "user";
const queryAttachments = message.attachments.filter(isQueryAttachment);
const html = useMemo(
() => (message.content ? (marked.parse(message.content) as string) : ""),
() =>
message.content
? DOMPurify.sanitize(marked.parse(message.content) as string)
: "",
[message.content],
);

Expand Down
3 changes: 2 additions & 1 deletion packages/appkit/src/plugins/server/remote-tunnel/remote-tunnel-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,8 @@ export class RemoteTunnelManager {
}

res.cookie("dev-tunnel-id", tunnelId, {
httpOnly: false,
httpOnly: true,
secure: true,
sameSite: "lax",
});

Expand Down
7 changes: 7 additions & 0 deletions packages/appkit/tsdown.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,12 @@ export default defineConfig([
return /^[^./]/.test(id) || id.includes("/node_modules/");
},
tsconfig: "./tsconfig.json",
copy: [
Copy link
Copy Markdown
Member Author

@pkosiec pkosiec Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was needed to resolve an issue that crashed the app when running the tunnel:

node:fs:442
    return binding.readFileUtf8(path, stringToFlags(options.flag));
                   ^

Error: ENOENT: no such file or directory, open '/app/python/source_code/node_modules/@databricks/appkit/dist/plugins/server/remote-tunnel/index.html'
    at Object.readFileSync (node:fs:442:20)
    at file:///app/python/source_code/node_modules/@databricks/appkit/dist/plugins/server/remote-tunnel/remote-tunnel-manager.js:121:18
    at middleware (file:///app/python/source_code/node_modules/@databricks/appkit/dist/plugins/server/remote-tunnel/remote-tunnel-controller.js:60:51) {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: '/app/python/source_code/node_modules/@databricks/appkit/dist/plugins/server/remote-tunnel/index.html'
}

Node.js v22.16.0

{
from: "src/plugins/server/remote-tunnel/*.html",
to: "dist/plugins/server/remote-tunnel",
flatten: true,
},
],
},
]);
12 changes: 8 additions & 4 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading