P0: isolate Sheets caches and recover after entitlement upgrades - #33
Conversation
|
Warning Review limit reached
Next review available in: 39 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughRuntime 1.3.1 adds spreadsheet-isolated cache handling, removes legacy unscoped API-key fallback, expands quota responses, updates release records, and adds a Node.js secret scanner to test and validation workflows. ChangesRuntime cache isolation and release
Repository secret scanning
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ConnectionTest
participant CacheContext
participant CacheStore
participant ExternalAPI
ConnectionTest->>ExternalAPI: validate API key
ExternalAPI-->>ConnectionTest: connection response
ConnectionTest->>CacheContext: invalidate active generation
CacheContext->>CacheStore: refresh scoped generation
CacheStore-->>CacheContext: refreshed scope
CacheContext-->>ConnectionTest: invalidation result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Code.gs (1)
781-790: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSeparate the two digests in the spreadsheet namespace.
stableCacheDigest_returns base36 of a 32-bit value, so its length varies from 1 to 7 characters. Concatenating two variable-length digests is ambiguous. Two different spreadsheet IDs can produce digest pairs such as("ab","cde")and("abc","de"), and both yieldabcde. A collision makes two spreadsheets of the same user share cached responses and entitlement blocks, which defeats the isolation this change adds.Add a separator so the namespace keeps the full entropy of both digests.
♻️ Proposed fix
const spreadsheetHash = - `${stableCacheDigest_(spreadsheetId)}${stableCacheDigest_(`sheet:${spreadsheetId}`)}`; + `${stableCacheDigest_(spreadsheetId)}x${stableCacheDigest_(`sheet:${spreadsheetId}`)}`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Code.gs` around lines 781 - 790, Update namespacedCacheKey_ so the two stableCacheDigest_ values in the user-scoped spreadsheet hash are joined with an unambiguous separator rather than concatenated directly. Preserve the existing digest inputs and cache-key structure while ensuring different digest pairs cannot produce the same combined namespace.scripts/scan-secrets.js (1)
6-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBare 64-hex-char pattern lacks context, unlike the other patterns.
PATTERNS[0]at Line 7 matches any 64-character hex string with no surrounding keyword or prefix requirement. Every other pattern in this list (Lines 8-11) requires a recognizable prefix (sk-,AIza,gh[pousr]_) or keyword (api_key,access_token,secret). A bare hex-64 match will also fire on unrelated content such as SHA-256 checksums, git blob hashes, or other generated hash identifiers, none of which are secrets.The PR objectives introduce an "opaque active-spreadsheet hash" for cache namespacing. If that hash format is ever documented with a literal example value in a scanned text file, this pattern will flag it as a false positive and break the
validatepipeline.Add a keyword/context requirement to this pattern (similar to Line 11), or move it into a lower-confidence secondary check that only fires alongside a nearby keyword.
Run the following to confirm no tracked file already contains an unrelated bare 64-hex-char string that would trip this pattern:
#!/bin/bash rg -nP '\b[0-9a-fA-F]{64}\b' --hidden \ -g '!node_modules' -g '!.git' -g '!package-lock.json' \ -g '!scripts/scan-secrets.js' -g '!scripts/scan-secrets.sh' -g '!test/secret-scan.test.js' .🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/scan-secrets.js` around lines 6 - 12, The first entry in PATTERNS matches arbitrary 64-character hexadecimal strings, causing hash-like identifiers to be treated as secrets. Replace that bare pattern with a context-aware expression requiring a nearby secret-related keyword, or move it into a secondary check that only reports matches when such context is present; preserve the existing detection behavior for the prefixed and keyword-based patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/public-claims.test.js`:
- Around line 63-75: Update the assertions in the public claims test to validate
each release relationship within its source document: assert deployment ties
Apps Script version 12 to release 1.3.0 and its non-publication status, and
assert listing ties version 11 to release 1.2.2. Remove the cross-document
records check and keep the existing current-version assertions.
---
Nitpick comments:
In `@Code.gs`:
- Around line 781-790: Update namespacedCacheKey_ so the two stableCacheDigest_
values in the user-scoped spreadsheet hash are joined with an unambiguous
separator rather than concatenated directly. Preserve the existing digest inputs
and cache-key structure while ensuring different digest pairs cannot produce the
same combined namespace.
In `@scripts/scan-secrets.js`:
- Around line 6-12: The first entry in PATTERNS matches arbitrary 64-character
hexadecimal strings, causing hash-like identifiers to be treated as secrets.
Replace that bare pattern with a context-aware expression requiring a nearby
secret-related keyword, or move it into a secondary check that only reports
matches when such context is present; preserve the existing detection behavior
for the prefixed and keyword-based patterns.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 73850c2a-372e-4c91-b03b-b5a87e3d34b0
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
Code.gsDEPLOYMENT_GUIDE.mdMARKETPLACE_LISTING.mdOAUTH_VERIFICATION.mdREADME.mddocs/index.htmlpackage.jsonscripts/scan-secrets.jsscripts/scan-secrets.shtest/public-claims.test.jstest/runtime.test.jstest/secret-scan.test.jstest/validate_code.js
|
Follow-up
|
1 participant
Summary
Red / green proof
RED before runtime fix:
GREEN after fix:
npm run validate: 88/88 tests passgit diff --checkpassesRelease gate
The public Marketplace configuration remains pinned to immutable Apps Script version 11 (runtime 1.2.2). Immutable version 12 (runtime 1.3.0) was created but failed this cache-isolation review and was never published. Do not cut/select/publish the 1.3.1 replacement until this exact head is hosted-green, independently reviewed, and passes an installed-account disposable-spreadsheet formula/recovery smoke.
Summary by CodeRabbit
Bug Fixes
Security
Documentation