Fix security vulnerabilities: update deps and harden code#81
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughPinned CI action refs to commit SHAs, limited Babel's Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.16.5)app/editor-webview/page.tsx┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m core/enex/converter.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying everfreenote with
|
| Latest commit: |
a8c10e1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://00002661.everfreenote.pages.dev |
| Branch Preview URL: | https://claude-angry-golick.everfreenote.pages.dev |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
core/services/sanitizer.ts (1)
27-34: Misleading comment aboutKEEP_CONTENToption.The two-pass sanitization approach is correct and more robust than regex-based stripping. However, the comment on line 28 references
KEEP_CONTENT: false, which is not a DOMPurify option being used here. The actual mechanism preventing content leakage isFORBID_CONTENTS, which is correctly configured on line 31.📝 Suggested comment fix
- // First pass: use DOMPurify to remove dangerous tags AND their content. - // KEEP_CONTENT: false ensures script/style inner text doesn't leak. + // First pass: use DOMPurify to remove dangerous tags AND their content. + // FORBID_CONTENTS ensures script/style inner text doesn't leak into output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/services/sanitizer.ts` around lines 27 - 34, The comment above the two-pass DOMPurify sanitization is misleading: replace the reference to the non-existent KEEP_CONTENT option with the actual mechanism used, FORBID_CONTENTS, and clarify that FORBID_TAGS plus FORBID_CONTENTS prevent script/style inner text leakage; update the comment near the DOMPurify.sanitize calls in sanitizer.ts (the block that creates `safe` and then strips tags) to mention FORBID_CONTENTS rather than KEEP_CONTENT.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Line 31: The workflow uses mismatched pinned action refs: update the three
action pins so the commit SHAs match the claimed versions (or adjust claimed
versions to match the SHAs). Specifically, for actions/checkout (currently
"actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 // v4"), replace the
SHA with the correct v4 commit or change the tag to a valid v4 ref; for
denoland/setup-deno (currently pinned to a SHA but claims v2.0.4), either set
the SHA that corresponds to v2.0.3 (the actual latest release) or change the
claimed version to the SHA’s real release; and for gitleaks/gitleaks-action
(currently pinned to a SHA while claiming v2), swap in the correct commit SHA
for the v2 release or update the version label to the SHA’s actual release.
Ensure each updated reference uses the canonical commit SHA that matches the
named version tag so the action pin and claimed version are consistent.
In @.github/workflows/claude.yml:
- Line 29: The workflow uses pinned SHAs for actions that are
unverifiable/outdated; update the action references to use current released
semantic tags or verified SHAs: replace
actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 with the official
release tag (e.g., actions/checkout@v4 or a verified v4.x.y SHA) and replace
anthropics/claude-code-action@e69ec4d5e5868930cecb8da7360a51fa9c4de5d6 with the
published tag (e.g., anthropics/claude-code-action@v1 or a verified v1.x.y SHA);
ensure you reference the official GitHub release/commit for each action before
updating to confirm the SHA matches the public source.
In `@core/enex/converter.ts`:
- Around line 165-170: The chained string.replace calls using the regexes like
/^(?:\s*<p[^>]*>(?:\s| | | |<br\s*\/?>)*<\/p>\s*)+/gi and
/(?:\s*<p[^>]*>(?:\s| | | |<br\s*\/?>)*<\/p>\s*)+$/gi are
vulnerable to ReDoS; replace this whole regex-based cleanup with DOM parsing:
parse the ENML/HTML into a DOM (via DOMParser or isomorphic-dompurify’s
parse/sanitize facility), walk the document to remove leading/trailing empty <p>
nodes (where "empty" means only whitespace, nbsp entities or <br> children) and
normalize <p>...</p> that contain only optional whitespace and a single <br> to
<p></p>, then serialize the DOM back to a string; remove the string.replace
chain in converter.ts and implement the DOM-based logic so the three
transformations (remove leading empty <p>, remove trailing empty <p>, normalize
empty paragraphs) are done deterministically without regex backtracking.
---
Nitpick comments:
In `@core/services/sanitizer.ts`:
- Around line 27-34: The comment above the two-pass DOMPurify sanitization is
misleading: replace the reference to the non-existent KEEP_CONTENT option with
the actual mechanism used, FORBID_CONTENTS, and clarify that FORBID_TAGS plus
FORBID_CONTENTS prevent script/style inner text leakage; update the comment near
the DOMPurify.sanitize calls in sanitizer.ts (the block that creates `safe` and
then strips tags) to mention FORBID_CONTENTS rather than KEEP_CONTENT.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6fede441-9ee5-411f-9097-c1ec58729590
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
.babelrc.github/workflows/build.yml.github/workflows/claude.ymlapp/editor-webview/page.tsxcore/enex/converter.tscore/services/sanitizer.tscore/utils/editorWebViewBridge.tspackage.jsonui/mobile/utils/htmlToPlainText.ts
Update next (16.0.10→16.1.7), isomorphic-dompurify, markdown-it and add npm overrides for 14 transitive packages (hono, tar, flatted, serialize-javascript, dompurify, etc.) to resolve ~80 Dependabot alerts. Code fixes for CodeQL alerts: - editor-webview: add postMessage origin check and DOMPurify sanitize before innerHTML usage - editorWebViewBridge: validate transferId format and chunk index range to prevent property injection - sanitizer: replace regex-based script/style stripping with two-pass DOMPurify (FORBID_CONTENTS + ALLOWED_TAGS:[]) - htmlToPlainText: decode & last to prevent double-escaping - ENEX converter: use non-capturing groups in regexes to reduce ReDoS risk - GitHub Actions: pin all actions to commit SHAs - .babelrc: move istanbul plugin to test env only (required for Next.js 16.1.7 Turbopack compatibility) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gitleaks-action and claude-code-action use annotated tags — the previous SHAs pointed to tag objects, not commits, which prevented the Build workflow from running. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The nested quantifiers in removeEmptyParagraphs regex caused CodeQL to flag exponential backtracking. Replace with an iterative loop that matches one paragraph at a time, avoiding nested repetition entirely. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
koreyba
force-pushed
the
claude/angry-golick
branch
from
afe8272 to
137871c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/utils/editorWebViewBridge.ts (1)
57-82:⚠️ Potential issue | 🟠 MajorDon't keep or commit partial chunk transfers.
Line 80 joins sparse arrays, so a missing chunk is silently treated as
''and truncated payloads are accepted as complete. Separately, started transfers never expire, so a long-lived store can grow without bound if a sender starts transfers and never reaches_CHUNK_END. Reject incomplete buffers on finalize and add TTL / max-outstanding cleanup for abandoned entries.Minimal guard for incomplete `_CHUNK_END` payloads
if (type.endsWith(CHUNK_END_SUFFIX)) { const p = payload as Partial<ChunkEndPayload> if (!isValidTransferId(p.transferId)) return null const entry = store[p.transferId] if (!entry) return null + for (let index = 0; index < entry.total; index += 1) { + if (typeof entry.chunks[index] !== 'string') { + delete store[p.transferId] + return null + } + } const text = entry.chunks.join('') delete store[p.transferId] return { baseType, text } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/utils/editorWebViewBridge.ts` around lines 57 - 82, The CHUNK handling currently accepts incomplete transfers (line joining sparse arrays) and never cleans up abandoned starts; update the chunk lifecycle in editorWebViewBridge.ts: when handling CHUNK_START_SUFFIX in the store entry add metadata (createdAt timestamp and expected total), enforce a global maxOutstandingTransfers limit and reject new starts when exceeded, and schedule or run a periodic cleanup that deletes entries older than TTL (e.g., createdAt + TTL) to prevent unbounded growth; when handling CHUNK_SUFFIX validate and record received indexes (or mark holes) and on CHUNK_END_SUFFIX ensure all entry.chunks indexes 0..total-1 are present (no undefined holes) before joining — if any missing, delete the entry and return null (reject finalize); reference the symbols CHUNK_START_SUFFIX, CHUNK_SUFFIX, CHUNK_END_SUFFIX, store, isValidTransferId and the handler that returns { baseType, text } so you implement the TTL, maxOutstandingTransfers, hole detection, and deletion on failure.app/editor-webview/page.tsx (1)
71-76:⚠️ Potential issue | 🔴 CriticalSanitize before the early return.
Line 73 returns the raw HTML when neither rewrite knob is configured, so the DOMPurify hardening only runs on the image-rewrite path. In the default/no-rewrite case,
SET_CONTENTcan still feed unsanitized HTML into the editor.Minimal fix
const { devHost, supabaseUrl } = getMobileConfig() const supabaseOrigin = normalizeOrigin(supabaseUrl) - if (!devHost && !supabaseOrigin) return html + const sanitized = DOMPurify.sanitize(html) + if (!devHost && !supabaseOrigin) return sanitized const container = document.createElement('div') - container.innerHTML = DOMPurify.sanitize(html) + container.innerHTML = sanitized🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/editor-webview/page.tsx` around lines 71 - 76, The current early return returns the raw html without sanitization; move the DOMPurify.sanitize call before the devHost/supabaseOrigin check so you always sanitize the input. Specifically, call DOMPurify.sanitize(html) into a sanitizedHtml variable before evaluating getMobileConfig()/normalizeOrigin(), use sanitizedHtml for the early return (instead of raw html) and for container.innerHTML, and keep references to devHost, supabaseOrigin, container, and DOMPurify consistent (so functions like getMobileConfig and normalizeOrigin remain unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/enex/converter.ts`:
- Around line 193-199: The outer html.replace call (the regex used in
html.replace at the top of the block) only matches when the last token before
</p> is a <br>, so cases like <p> <br> </p> are skipped; update that
outer regex (the first argument to html.replace) to allow optional trailing
whitespace/entities after the final <br> before </p> (mirroring the inner test
that allows (?:\s| | | )* around <br\s*\/?>), or alternatively
loosen the outer match to capture any <p...>...</p> and rely on the existing
inner replacement callback and its inner regex to decide whether to return an
empty <p${attrs}></p> for strings containing only whitespace/entities and <br>s;
apply this change to the html.replace call and keep the inner test (the inner
regex and callback) unchanged.
---
Outside diff comments:
In `@app/editor-webview/page.tsx`:
- Around line 71-76: The current early return returns the raw html without
sanitization; move the DOMPurify.sanitize call before the devHost/supabaseOrigin
check so you always sanitize the input. Specifically, call
DOMPurify.sanitize(html) into a sanitizedHtml variable before evaluating
getMobileConfig()/normalizeOrigin(), use sanitizedHtml for the early return
(instead of raw html) and for container.innerHTML, and keep references to
devHost, supabaseOrigin, container, and DOMPurify consistent (so functions like
getMobileConfig and normalizeOrigin remain unchanged).
In `@core/utils/editorWebViewBridge.ts`:
- Around line 57-82: The CHUNK handling currently accepts incomplete transfers
(line joining sparse arrays) and never cleans up abandoned starts; update the
chunk lifecycle in editorWebViewBridge.ts: when handling CHUNK_START_SUFFIX in
the store entry add metadata (createdAt timestamp and expected total), enforce a
global maxOutstandingTransfers limit and reject new starts when exceeded, and
schedule or run a periodic cleanup that deletes entries older than TTL (e.g.,
createdAt + TTL) to prevent unbounded growth; when handling CHUNK_SUFFIX
validate and record received indexes (or mark holes) and on CHUNK_END_SUFFIX
ensure all entry.chunks indexes 0..total-1 are present (no undefined holes)
before joining — if any missing, delete the entry and return null (reject
finalize); reference the symbols CHUNK_START_SUFFIX, CHUNK_SUFFIX,
CHUNK_END_SUFFIX, store, isValidTransferId and the handler that returns {
baseType, text } so you implement the TTL, maxOutstandingTransfers, hole
detection, and deletion on failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ace91f69-ce0a-4414-b3d3-7e8b5eb9a6a5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
.babelrc.github/workflows/build.yml.github/workflows/claude.ymlapp/editor-webview/page.tsxcore/enex/converter.tscore/services/sanitizer.tscore/utils/editorWebViewBridge.tspackage.jsonui/mobile/utils/htmlToPlainText.ts
✅ Files skipped from review due to trivial changes (4)
- .babelrc
- ui/mobile/utils/htmlToPlainText.ts
- .github/workflows/build.yml
- .github/workflows/claude.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- core/services/sanitizer.ts
- package.json
Replace /^\s*/ and /\s*$/ regex patterns with native trimStart()/trimEnd() to eliminate potential backtracking-based DoS flagged by SonarCloud. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix outer regex to allow trailing whitespace/entities after <br>: (?:[^<]*<br>)* → (?:[^<]|<br>)* so <p> <br> </p> is matched - Use replaceAll() instead of replace() per SonarCloud recommendation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- startsWith() instead of indexOf() !== 0 - replaceAll() instead of replace() for regex replacements - .at(-1) instead of [arr.length - 1] - globalThis instead of window Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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
next(16.0.10→16.1.7),isomorphic-dompurify,markdown-itand add npm overrides for 14 transitive packages to resolve ~80 Dependabot alertsChanges
Dependency updates (
package.json)next16.0.10 → 16.1.7 (6 CVE: DoS, CSRF bypass, request smuggling)isomorphic-dompurify2.34.0 → 2.36.0 (dompurify mutation-XSS fix)markdown-it14.1.0 → 14.1.1 (ReDoS)eslint-config-next+@next/eslint-plugin-nextsynced with nextCode security fixes
app/editor-webview/page.tsx: origin check on postMessage handler + DOMPurify.sanitize before innerHTMLcore/utils/editorWebViewBridge.ts: validate transferId (alphanumeric, max 200 chars) and chunk index boundscore/services/sanitizer.ts: replace unreliable regex-based script/style stripping with two-pass DOMPurify using FORBID_CONTENTSui/mobile/utils/htmlToPlainText.ts: decode&last to prevent double-decoding chain (&lt;→<→<)core/enex/converter.ts: replace capturing groups with non-capturing(?:...)in regexes to reduce ReDoS surface.github/workflows/: pin all actions to commit SHAs.babelrc: move istanbul plugin toenv.test(required for Next.js 16.1.7 Turbopack compat)Remaining (non-critical, dev-only)
minimatch3.1.2 in eslint — major breaking change, dev dependency only@tootallnate/once,ajv— via jest-environment-jsdom, no prod impactTest plan
npm run buildpassesnpm run test:unit— 28 suites, 299 tests passnpm run type-checkpassesnpm audit— down to 6 low/moderate (all dev-only)🤖 Generated with Claude Code
Summary by CodeRabbit
Security
Bug Fixes
Chores