web: add integrity check for marked CDN and cap highlight regex input - #109
Conversation
Summary of ChangesHello @lawyered0, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on two key areas: bolstering the security of the web UI by adding integrity checks for a third-party CDN script, and enhancing client-side stability by preventing potential performance degradation from overly complex or long user-provided regex queries. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces two important security and stability improvements. First, it adds Subresource Integrity (SRI) to the marked script loaded from a CDN, which prevents the execution of a compromised script. It also pins the script to a specific version, which is a best practice. Second, it limits the length of the search query used for client-side highlighting to mitigate the risk of Regular Expression Denial of Service (ReDoS) attacks with very long inputs. My feedback focuses on improving the maintainability of the query length limit.
| const normalizedQuery = query.slice(0, 100); | ||
| const queryEscaped = normalizedQuery.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); |
There was a problem hiding this comment.
To improve readability and maintainability, it's a good practice to avoid magic numbers. I suggest defining 100 as a constant. This suggestion also combines the slicing and escaping into a single line for conciseness. Ideally, the constant would be defined at the top of the file with other constants.
| const normalizedQuery = query.slice(0, 100); | |
| const queryEscaped = normalizedQuery.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | |
| const HIGHLIGHT_QUERY_MAX_LEN = 100; | |
| const queryEscaped = query.slice(0, HIGHLIGHT_QUERY_MAX_LEN).replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); |
|
Addressed the maintainability feedback: the query length magic number is now defined as a top-level constant and reused by memory search normalization/snippet/highlight paths in the latest commit. Please re-review when convenient; no functional changes besides this consistency cleanup. |
|
Follow-up patch in latest commit: add compatibility fallback in session DB load path. If is absent, we now read legacy , then proceed. This directly addresses issue #108 / legacy key auth failure without broad behavior changes. |
|
Follow-up patch in latest commit: add compatibility fallback in session DB load path. If |
b3f25c0 to
dbad950
Compare
ilblackdragon
left a comment
There was a problem hiding this comment.
Review: LGTM with minor suggestions
SRI for marked CDN (index.html)
The SRI change is solid:
- Pins
markedto a specific version (17.0.2) instead of floatinglatest, preventing supply-chain attacks via CDN compromise. - The
sha384integrity hash is verified correct -- I independently fetched the file and recomputed the digest. crossorigin="anonymous"is correctly set (required for SRI with cross-origin scripts).- The switch from
marked.min.jstomarked.umd.min.jsis correct for<script>tag usage (UMD exposes themarkedglobal thatapp.jsreferences at line 283).
One note: this jumps from the previously-unpinned version (which resolved to v15.0.12) to v17.0.2. That is a two-major-version bump. The call site (marked.parse(text)) is simple enough that it likely works fine, but worth being aware of if any rendering regressions appear.
Regex input cap (app.js)
The normalizeSearchQuery function and MEMORY_SEARCH_QUERY_MAX_LENGTH = 100 limit are a reasonable defense against ReDoS. The regex-escape already exists (replace(/[.*+?^${}()|[\]\\]/g, '\\$&')), but capping input length before escape+compile is a good belt-and-suspenders measure.
Minor observations (non-blocking):
-
Double normalization in
snippetAround: ThesearchMemoryfunction already normalizes the query before passing it tosnippetAround, butsnippetAroundnormalizes again internally. This is harmless (idempotent) but slightly redundant. The same applies tohighlightQuery-- the caller already passes a normalized query, but the function normalizes again. If these functions are only called fromsearchMemory, the internal normalization is unnecessary. That said, having the defensive check at both layers is fine for a small codebase like this. -
Consider adding
maxlength="100"to the HTML input element (<input type="text" id="memory-search">at line 78 ofindex.html). This gives the user immediate visual feedback about the limit and prevents typing beyond 100 chars, complementing the JS-side truncation. Not required, just a UX nicety. -
Trailing blank line removal (line 1044-1045 in the diff): The PR removes a blank line between
highlightQueryand the// --- Logs ---section comment. This is fine but cosmetic -- mentioning it only for completeness.
Approving -- both changes are clear security improvements with no functional regressions.
Batch 2 PR Review: nearai/ironclaw PRs 111, 110, 109, 103, 95, 74Reviewer: AI Sub-Agent PR #111: Fix backwards compatibility for nearai.session_tokenSummaryThis PR adds backwards compatibility for the nearai session management by implementing a fallback mechanism. When Pros
Concerns
Suggestions
PR #110: Add env docs for local LLM providersSummaryThis PR updates the Pros
Concerns
Suggestions
PR #109: Normalize memory search query and update marked.jsSummaryThis PR addresses two security and stability issues in the web interface. First, it adds input normalization for memory search queries to prevent excessive query lengths and invalid input types. Second, it updates the marked.js dependency to a specific version with integrity hashing for supply chain security. The changes are defensive in nature, preventing potential DoS attacks and ensuring the integrity of third-party JavaScript dependencies. Pros
Concerns
Suggestions
PR #103: Per-request model override for OpenAI-compatible APISummaryThis is a significant feature PR that adds per-request model override capability across the entire LLM provider ecosystem. Previously, all requests used the active model, but now clients can specify a different model per request. The changes span multiple modules: request structs now include optional Pros
Concerns
Suggestions
PR #95: Add Venice AI provider and embeddingsSummaryThis PR adds comprehensive support for Venice AI as both an LLM provider and an embeddings provider. It introduces a new Pros
Concerns
Suggestions
PR #74: Security fix: Enhanced HTTP response size validationSummaryThis PR strengthens the HTTP tool's defense against OOM attacks by implementing two-stage response size validation. First, it checks the Pros
Concerns
Suggestions
Overall RecommendationsHigh Priority
Medium Priority
Low Priority
General Observations
|
…nearai#109) * web: add integrity check for marked CDN and cap highlight regex input * web: normalize memory search query before snippet+highlight matching * web: place memory query length constant with top-level config --------- Co-authored-by: Clawyered <clawyered@macbookair.home> Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com>
…nearai#109) * web: add integrity check for marked CDN and cap highlight regex input * web: normalize memory search query before snippet+highlight matching * web: place memory query length constant with top-level config --------- Co-authored-by: Clawyered <clawyered@macbookair.home> Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com>
Summary
Validation
Notes