fix: redact credentials from a layer's connection record - #1704
Conversation
The egress redaction pass added in #1702 sweeps each layer's `source`, `metadata`, and `sourcePath`, but leaves `connection` untouched. `connection.lastError` is free-form text taken from a caught error, and a refresh path that words it as `Failed to fetch ${url}` would carry the request's credential parameters straight through Share, HTML export, embed, and collaboration. Today's built-in refresh paths only ever store generic status strings, so nothing leaks in practice — this closes the blind spot rather than a live bug, so the "no-secret guarantee" does not depend on how a future error message is phrased. - packages/core/src/credentials.ts: sweep `layer.connection` through `redactConfigurationValue` alongside the other three fields. - python/src/geolibre/project.py: the same field in the Python mirror. - Tests on both sides assert the token is stripped while the rest of the connection record (interval, timestamps) survives.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughCredential redaction now includes each layer’s ChangesLayer connection credential redaction
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. 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}
|
This confirms the test is well-formed and mirrors the TS test correctly. Given the small, well-scoped nature of this PR and the thorough verification, I don't have any inline comments to post — I did not find bugs, security issues, or CLAUDE.md violations. The one thing worth noting (very low severity, not worth an inline comment) is that the PR description claims the tests assert Code reviewBugs: None found. Traced Security: This closes a real gap — Performance: No concerns — the added sweep is O(1) extra field per layer, using the existing recursive redactor. Confidence: high. Quality: TS and Python implementations are kept symmetric, consistent with the existing CLAUDE.md: No violations — this doesn't touch any of the mirrored-constant modules the guidelines call out (Whitebox catalog, PMTiles zoom caps, remote file size limits, etc.), and no lockfile or CSP changes are involved. |
🔍 Cloudflare PR preview
|
There was a problem hiding this comment.
Pull request overview
Closes a remaining credential-redaction gap by ensuring each layer’s persisted connection record (notably connection.lastError, which can contain request URLs) is swept through the same redaction logic as other layer configuration, preventing secrets from leaking via Share/HTML export/embed/collaboration and Python serialization.
Changes:
- Redact
layer.connectionviaredactConfigurationValuein@geolibre/core’sredactProjectCredentials. - Mirror the same sweep in the Python project redaction helper.
- Add JS + Python tests asserting query-token removal while preserving non-sensitive connection fields.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/core/src/credentials.ts | Adds layer.connection to the redaction sweep using the existing configuration redactor. |
| tests/project-credentials.test.ts | Adds a regression test ensuring connection.lastError cannot carry credential query params out of the project. |
| python/src/geolibre/project.py | Extends Python redaction to sweep layer["connection"] alongside existing layer fields. |
| python/tests/test_scripting.py | Adds a Python regression test validating redaction of secrets from connection.lastError. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "interval": 300, | ||
| "lastSyncedAt": "2026-01-01T00:00:00.000Z", | ||
| "lastError": "Failed to fetch https://example.com/tiles?token=py-connection-secret", | ||
| "onFailure": "keep-last", |
| # `connection.lastError` is free-form text taken from a caught | ||
| # error, which a future refresh path could easily build from the | ||
| # request URL. Sweeping it costs nothing and keeps the no-secret | ||
| # guarantee from depending on how an error message is worded. | ||
| for field in ("source", "metadata", "sourcePath", "connection"): | ||
| if field in layer: | ||
| layer[field] = _redact_config(layer[field]) |
🔍 GitHub Pages PR preview
|
Follow-up to #1702, closing the last gap the review raised there (that comment landed after the PR was merged).
The gap
redactProjectCredentialssweeps each layer'ssource,metadata, andsourcePath, butlayer.connectionpasses through untouched.connection.lastErroris free-form text taken from a caught error, and a refresh path that words it as`Failed to fetch ${url}`— a common pattern — would carry the request's?token=…straight through Share, HTML export, embed, and collaboration.Today's built-in refresh paths (
layer-refresh.ts) only ever store generic strings like"Request failed with status 403", so nothing leaks in practice. This closes the blind spot so the module's no-secret guarantee does not depend on how a future error message happens to be phrased.Changes
packages/core/src/credentials.ts— sweeplayer.connectionthroughredactConfigurationValuealongside the other three fields.python/src/geolibre/project.py— the same field in the Python mirror, which had the identical gap.onFailure) survives.Verification
npm run test:frontend— 5056 passing (2 new)python -m pytestinpython/— 175 passing (1 new)pre-commit run --files <changed>— cleanSummary by CodeRabbit
Bug Fixes
Tests