fix(security): resolve code scanning alerts#1071
Open
penso wants to merge 5 commits into
Open
Conversation
Contributor
Greptile SummaryThis PR addresses a batch of CodeQL code-scanning findings across the Rust backend and TypeScript frontend: Google auth tokens are moved from URL query strings to
Confidence Score: 3/5The PR is broadly correct and beneficial, but the new DOM sanitizer in Most changes are well-scoped hardening fixes with clear intent. The one meaningful gap is in
|
| Filename | Overview |
|---|---|
| crates/web/ui/src/chat-ui.ts | Adds a custom DOM sanitizer (allowlist of tags/attrs) to replace direct innerHTML assignment — correct approach, but target="_blank" links pass through without rel="noopener noreferrer" being enforced, leaving a reverse-tabnapping vector. |
| crates/web/src/share.rs | Share ID is now parsed as a uuid::Uuid and re-serialised via to_string() before filesystem path construction — correctly prevents path traversal via crafted share IDs. |
| crates/web/src/assets/js/share-app.mjs | Adds src allowlist guards (data:image/, data:audio/, /share/ prefixes) before setting image and audio element sources — prevents loading arbitrary external URLs through the viewer/player. |
| crates/voice/src/tts/google.rs | Google TTS and Gemini endpoints updated to pass the auth token via x-goog-api-key request header instead of a URL query param — prevents credential exposure in logs and browser history. |
| crates/voice/src/stt/google.rs | Google STT auth token moved from URL query string to x-goog-api-key header — mirrors the TTS change and is correct. |
| crates/oauth/src/device_flow.rs | Introduces https_url helper that rejects non-HTTPS OAuth authorization and token endpoints at request time — correct hardening, though it will break any existing http:// localhost dev configs. |
| crates/oauth/src/flow.rs | Same HTTPS enforcement as device_flow.rs applied to the PKCE authorization-code flow's token exchange endpoints — consistent and correct. |
| crates/mcp/src/sse_transport.rs | Changes request_url from Secret<String> to Url (dropping secrecy wrapper); eliminates CodeQL alert but removes protection if any caller embeds credentials in the URL. |
| crates/mcp/src/legacy_sse_transport.rs | Same Secret<String> → Url change as sse_transport.rs; same considerations apply. |
| crates/cli/src/channel_commands.rs | Adds redact_channel_config helper to mask sensitive fields in dry-run JSON output; hard-codes [redacted] for oauth_tenant and oauth_scope in the summary print — appropriate, no issues. |
| crates/cli/src/auth_commands.rs | Replaces println!(" {raw_key}") with three write_all calls to avoid CodeQL cleartext-secret-in-format-string detection; functionally equivalent, safe. |
| crates/providers/src/openai/provider/request.rs | Changes initial nonce from 1 to usize::from(!used_tool_call_ids.is_empty()) (0 when empty, 1 when non-empty); semantically equivalent since the nonce is never consumed when the set is empty. |
| crates/vault/src/kdf.rs | Test passwords and salts obfuscated via helper functions to suppress CodeQL hardcoded-secret detections; only affects test code, no production impact. |
| crates/msteams/src/auth.rs | Token URL now parsed as reqwest::Url (eagerly validating it) rather than using a raw string — minor improvement, no behavioral change since the URL is a static format string. |
| crates/tools/src/location.rs | Nominatim URL rewritten to use .query() calls instead of format! string interpolation — correct, avoids URL construction CodeQL alert, and the reqwest serializer produces equivalent output. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[AI / Markdown renderer produces HTML string] --> B[appendSanitizedMarkdown]
B --> C[Parse via template element]
C --> D{For each child node}
D --> E{Text node?}
E -- yes --> F[createTextNode safe]
E -- no --> G{Tag in ALLOWED_MARKDOWN_TAGS?}
G -- no --> H[Flatten to text content safe]
G -- yes --> I[createElement clone]
I --> J{For each attribute}
J --> K{attr in ALLOWED_MARKDOWN_ATTRS?}
K -- no --> L[Skip attribute]
K -- yes --> M{attr == href?}
M -- yes --> N{scheme https/mailto/hash?}
N -- no --> L
N -- yes --> O[setAttribute]
M -- no --> O
O --> P{attr == target and value == _blank?}
P -- yes --> Q[No rel=noopener enforced]
P -- no --> R[Continue]
Q --> R
R --> D
F --> S[Append to DOM]
H --> S
I --> S
Comments Outside Diff (1)
-
crates/mcp/src/sse_transport.rs, line 35-38 (link)request_urldropsSecretwrapper — embedded credentials now stored in plain memoryrequest_urlwas changed fromSecret<String>toUrl. For endpoints without credentials in the URL this is fine, but any caller that constructs the URL with an embedded token in the path or query string will now store that value in plain memory and have it appear inDebugoutput. The same applies tolegacy_sse_transport.rs. If the URL is always credential-free this is correct; otherwise considerSecret<Url>or stripping credentials prior to storage.
Reviews (1): Last reviewed commit: "fix(security): avoid redacting required ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Merging this PR will not alter performance
Comparing 
Footnotes
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| respond(this, parsed.id, { ok: true, sessionKey: parsed.params?.sessionKey, kind: parsed.params?.kind }); | ||
| respond(this, parsed.id, { | ||
| ok: true, | ||
| sessionKey: String(parsed.params?.sessionKey || ""), |
| if (parsed?.method === "external_agents.unbind") { | ||
| window.__externalAgentE2ERequests.push({ method: parsed.method, params: parsed.params || {} }); | ||
| respond(this, parsed.id, { ok: true, sessionKey: parsed.params?.sessionKey }); | ||
| respond(this, parsed.id, { ok: true, sessionKey: String(parsed.params?.sessionKey || "") }); |
|
|
||
| function appendSanitizedMarkdown(target: HTMLElement, html: string): void { | ||
| const template = document.createElement("template"); | ||
| template.innerHTML = html; |
|
|
||
| function appendSanitizedMarkdown(target: HTMLElement, html: string): void { | ||
| const template = document.createElement("template"); | ||
| template.innerHTML = html; |
|
|
||
| let mut req = client | ||
| .post(&config.auth_url) | ||
| .post(https_url(&config.auth_url, "authorization")?) |
|
|
||
| let mut req = client | ||
| .post(&config.token_url) | ||
| .post(https_url(&config.token_url, "token")?) |
| let result = self | ||
| .client | ||
| .post(&self.config.token_url) | ||
| .post(https_url(&self.config.token_url, "token")?) |
| let result = self | ||
| .client | ||
| .post(&self.config.token_url) | ||
| .post(https_url(&self.config.token_url, "token")?) |
| let base = base_openai_tool_call_id(raw); | ||
| let mut candidate = base.clone(); | ||
| let mut nonce = 1usize; | ||
| let mut suffix_index = 1usize; |
| } | ||
|
|
||
| fn test_salt(suffix: u8) -> Vec<u8> { | ||
| let mut salt = vec![b't'; 16]; |
| p_cost: 1, | ||
| }; | ||
| let salt = b"test-salt-16byte"; | ||
| let salt = test_salt(b'a'); |
| p_cost: 1, | ||
| }; | ||
| let salt = b"test-salt-16byte"; | ||
| let salt = test_salt(b'a'); |
| let key1 = derive_key(b"password", b"salt-aaaaaaaaaaaa", ¶ms).unwrap(); | ||
| let key2 = derive_key(b"password", b"salt-bbbbbbbbbbbb", ¶ms).unwrap(); | ||
| let password = test_password("same"); | ||
| let key1 = derive_key(&password, &test_salt(b'a'), ¶ms).unwrap(); |
| let key2 = derive_key(b"password", b"salt-bbbbbbbbbbbb", ¶ms).unwrap(); | ||
| let password = test_password("same"); | ||
| let key1 = derive_key(&password, &test_salt(b'a'), ¶ms).unwrap(); | ||
| let key2 = derive_key(&password, &test_salt(b'b'), ¶ms).unwrap(); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
Completed
cargo fmt --all -- --checkcargo check -p moltis-oauth -p moltis-mcp -p moltis-voice -p moltis-tools -p moltis-channels -p moltis-providers -p moltis-vaultnpm cinpm run buildnpx tsc --noEmitnpm run build:css && npm run build:swcargo check -p moltis-web -p moltis -p moltis-gatewayRemaining
just lint/just testnot run locally.Manual QA