Redact sensitive credentials from API responses#305
Open
nitrobass24 wants to merge 2 commits intodevelopfrom
Open
Redact sensitive credentials from API responses#305nitrobass24 wants to merge 2 commits intodevelopfrom
nitrobass24 wants to merge 2 commits intodevelopfrom
Conversation
- /server/config/get now returns "********" for remote_password and api_key instead of plaintext values - /server/config/set no longer echoes sensitive values in responses - Frontend OptionComponent skips sending updates when password field value equals the redacted sentinel (prevents overwriting stored password with "********") - Added Config.REDACTED_SENTINEL, Config.is_sensitive(), and Config.sensitive_property_names() for centralized sensitivity checks - 5 new tests (3 unit, 2 integration) Closes #257 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdd redaction of sensitive configuration values: backend tags specific fields as sensitive and returns a sentinel Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Frontend)
participant Server as API Server
participant Serializer as SerializeConfig
participant ConfigStore as Config (in-memory / persistence)
Client->>Server: GET /server/config/get
Server->>Serializer: SerializeConfig.config(inner_config)
Serializer->>ConfigStore: read all sections/options
alt option is sensitive
Serializer->>Serializer: replace value with "********"
end
Serializer-->>Server: JSON with redactions
Server-->>Client: 200 OK + redacted JSON
Client->>Server: POST/PUT /server/config/set/<section>/<option>/<value>
Server->>ConfigStore: verify section/option exists
alt value == "********" and option is sensitive
Server-->>Client: 400 Bad Request (reject sentinel)
else
Server->>ConfigStore: set_property(section, option, value)
alt option is sensitive
Server-->>Client: 200 OK ("section.option updated")
else
Server-->>Client: 200 OK ("section.option set to <value>")
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/angular/src/app/pages/settings/option.component.ts`:
- Around line 57-62: The check in option.component.ts uses the magic string
'********' to detect the backend redaction sentinel (used when
OptionType.Password), which duplicates backend Config.REDACTED_SENTINEL; extract
that literal into a named constant (e.g., REDACTED_SENTINEL) exported from a
shared config/module or at least a single constant in this component, then
replace the inline string check (the if referencing OptionType.Password) to
compare against REDACTED_SENTINEL and add a short comment documenting the
coupling to the backend sentinel so future changes are noticed.
In `@src/python/common/config.py`:
- Around line 468-478: The method sensitive_property_names is declared as a
`@classmethod` but never uses cls; change it to a `@staticmethod` to reflect that it
does not depend on class state: replace the `@classmethod` decorator with
`@staticmethod` for the function sensitive_property_names and keep its return
value and signature (no self/cls parameter), ensuring any callers still access
it as ConfigClass.sensitive_property_names() or via an instance if used
elsewhere.
In `@src/python/web/handler/config.py`:
- Around line 39-43: When updating config in the handler, add backend validation
to reject the redaction sentinel '********' for sensitive fields: before calling
inner_config.set_property in the code path where Config.is_sensitive(section,
key) is true, check if value == '********' and return an error HTTPResponse
(e.g., 400 with a message like "Cannot set sensitive field to redacted
sentinel") instead of persisting; this ensures inner_config.set_property and the
handler do not overwrite real secrets with the literal sentinel.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 48df2dd1-15a4-4a85-a65f-fe9ddfe3c1f0
📒 Files selected for processing (6)
src/angular/src/app/pages/settings/option.component.tssrc/python/common/config.pysrc/python/tests/integration/test_web/test_handler/test_config.pysrc/python/tests/unittests/test_web/test_serialize/test_serialize_config.pysrc/python/web/handler/config.pysrc/python/web/serialize/serialize_config.py
Comment on lines
+57
to
+62
| // Don't send updates for password fields when the value is the redacted sentinel. | ||
| // The server returns "********" for sensitive fields; sending it back would | ||
| // overwrite the real password with the sentinel. | ||
| if (this.type() === OptionType.Password && value === '********') { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting the sentinel constant to avoid magic string duplication.
The logic correctly prevents sending the redacted sentinel '********' back to the server, which would overwrite the real password. However, this string literal is duplicated between the frontend and backend (Config.REDACTED_SENTINEL in Python).
If the sentinel value ever changes on the backend, this frontend check would silently break. Consider extracting this to a shared constant or documenting the coupling.
Also note: if a user intentionally enters '********' as their actual password, the change will be silently ignored. This is an acceptable security trade-off but worth documenting.
💡 Suggested improvement
export enum OptionType {
Text,
Checkbox,
Password,
Select,
}
+/** Sentinel value used by the server to represent redacted sensitive fields. */
+const REDACTED_SENTINEL = '********';
+
`@Component`({Then use REDACTED_SENTINEL in the check:
- if (this.type() === OptionType.Password && value === '********') {
+ if (this.type() === OptionType.Password && value === REDACTED_SENTINEL) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/angular/src/app/pages/settings/option.component.ts` around lines 57 - 62,
The check in option.component.ts uses the magic string '********' to detect the
backend redaction sentinel (used when OptionType.Password), which duplicates
backend Config.REDACTED_SENTINEL; extract that literal into a named constant
(e.g., REDACTED_SENTINEL) exported from a shared config/module or at least a
single constant in this component, then replace the inline string check (the if
referencing OptionType.Password) to compare against REDACTED_SENTINEL and add a
short comment documenting the coupling to the backend sentinel so future changes
are noticed.
- Reject setting sensitive fields to "********" (the redacted sentinel) with 400 error — prevents accidentally overwriting real credentials via direct API access - Change sensitive_property_names from classmethod to staticmethod (cls was unused) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
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
1 participant
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
/server/config/getno longer returns plaintext passwords and API keys. Sensitive fields are replaced with"********"in API responses.Changes
SerializeConfig.config()redactsremote_passwordandapi_keyvalues/server/config/setno longer echoes sensitive values in responsesOptionComponentskips sending updates when a password field value equals the redacted sentinelConfig.REDACTED_SENTINEL,Config.is_sensitive(),Config.sensitive_property_names()What this means
Before:
GET /server/config/getreturned{"lftp": {"remote_password": "actual-password", ...}}After:
GET /server/config/getreturns{"lftp": {"remote_password": "********", ...}}Tests
Closes #257
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests