fix(credentials): raise when sentinel placeholder hits an invalid element#797
fix(credentials): raise when sentinel placeholder hits an invalid element#797leo-notte wants to merge 2 commits into
Conversation
…ment When a known credential sentinel (e.g. PASSWORD = "mycoolpassword") was supplied for a fill action but the targeted element failed the field's validate_element check, replace_credentials silently logged at trace and returned the action unmodified — causing the literal placeholder string to be typed into the form. This was easy to hit by targeting a <label> instead of its associated <input type="password">: UserNameField validates True unconditionally, so username substitution worked, but PasswordField requires attrs.type == "password", so password substitution silently no-op'd while the fill itself appeared to succeed. Replace the silent skip with a new CredentialFieldValidationError. Also add a dedicated except in NotteSession._action_with_vault so the broad `except ValueError` (which would otherwise swallow it via NotteBaseError) doesn't suppress the new error. Co-Authored-By: Claude Opus 4.7 (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: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughA new exception, CredentialFieldValidationError, was added to errors/processing and is raised by BaseVault.replace_credentials when a credential placeholder targets an element that fails the field’s validate_element check. The browser session now explicitly imports and re-raises CredentialFieldValidationError in _action_with_vault rather than treating it like a generic ValueError. Tests were added to assert that password placeholders on invalid elements raise the new error, valid password inputs substitute correctly, username placeholders remain substitutable, and the session layer propagates the validation error. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
| Filename | Overview |
|---|---|
| packages/notte-core/src/notte_core/errors/processing.py | New CredentialFieldValidationError class with dev/agent/user messages; well-structured, consistent with existing error classes. |
| packages/notte-core/src/notte_core/credentials/base.py | Silent logger.trace replaced with raise CredentialFieldValidationError in the sentinel-validation-failure branch; correctly scoped to only the LLM-chosen element path (not the regex/FormFill path). |
| packages/notte-browser/src/notte_browser/session.py | Adds except CredentialFieldValidationError: raise ahead of the broad except ValueError to prevent the new error (a ValueError subclass) from being silently swallowed. |
| tests/test_session_vault_helpers.py | 4 new unit tests covering the error raise, happy path, asymmetric username design, and session-level propagation; no integration test against a real browser/vault as required by repo policy for bug fix PRs. |
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
tests/test_session_vault_helpers.py:80-150
**Missing integration test for a bug fix PR**
The four new tests all use `MockVault` and a stubbed locator — they never exercise a real browser or real vault. Per the repo's test policy, every bug fix PR should include at least one integration test. The original repro (`the-internet.herokuapp.com/login` with a live vault entry) is listed in the test plan but remains unchecked, and the issue was only discovered because a user manually inspected `.value` via `eval-js`. An integration test that drives a real password field with a vault-backed sentinel would guard against regressions where the locator resolution silently returns a wrong element type again.
- Add a comment if the PR does n... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
Reviews (1): Last reviewed commit: "fix(credentials): raise when sentinel pl..." | Re-trigger Greptile
The docs-sdk-generate pre-commit hook was failing CI because the upstream OpenAPI spec at api.notte.cc gained a Search Web endpoint that wasn't reflected in the committed llms.txt. Regenerate to clear the hook. Co-Authored-By: Claude <noreply@anthropic.com>
|
🌊 Waiting for deployment on commit Detected prompts matching ⏳ Getting Started — current: no baseline yet · View metricsPrompt text:
Evaluating agent experience using 2027.dev · View dashboard |
Summary
When a known credential sentinel (e.g.
PASSWORD = "mycoolpassword"fromnotte_core.credentials) was supplied for aFillActionbut the targeted element failed the credential field'svalidate_elementcheck,replace_credentialssilently logged at trace level and returned the action unmodified — causing the literal placeholder string to be typed into the form.This was easy to hit by targeting a
<label>rather than its associated<input type="password">(e.g. via observe IDs that point at the label):UserNameField.validate_elementreturnsTrueunconditionally → username substitution proceeded.PasswordField.validate_elementrequiresattrs.type == "password"→ password substitution silently no-op'd while the fill itself reported success.End result:
mycoolpasswordgot typed into the password field as plaintext and the user only noticed because they manually re-read.valueviaeval-js.Changes
notte_core.errors.processing: newCredentialFieldValidationError(NotteBaseError)with dev/agent/user messages naming the placeholder, credential key, and offending element attrs.notte_core.credentials.base.replace_credentials: replaces the silentlogger.tracewith a raise. The branch only fires when the value already matched a known sentinel (lines 658–661 still raiseInvalidPlaceholderErrorfor unknown values), so raising here is the correct signal: the caller clearly intended a substitution and the literal sentinel must not leak into typing.notte_browser.session._action_with_vault: adds a dedicatedexcept CredentialFieldValidationError: raiseahead of the broadexcept ValueError, otherwise the new error would be swallowed (sinceNotteBaseErrorextendsValueError).tests/test_session_vault_helpers.py: 4 new tests covering the new raise, the happy substitute path, the asymmetric username path, and propagation through the session-level catch.Out of scope
validate_element(username unconditionally accepts, password requirestype=="password") is preserved — pinned by the newtest_username_placeholder_unaffected_by_validation_change. Tightening username validation would be a separate behavioural change.LocatorAttributeswould also fix the reported case but is a larger refactor; the raise is the safer first step because it surfaces the failure instead of corrupting the field.Test plan
pytest tests/test_session_vault_helpers.py— 8 passed (4 existing + 4 new)notte page fill M2 mycoolpasswordonthe-internet.herokuapp.com/loginwith a vault entry) and confirm the call now raises instead of silently typingmycoolpasswordinto the password field🤖 Generated with Claude Code
Summary by CodeRabbit
Note
Replaces a silent
logger.traceno-op inreplace_credentialswith a newCredentialFieldValidationErrorwhen a known credential sentinel targets an element that failsvalidate_element. Adds a dedicatedexcept CredentialFieldValidationError: raisein_action_with_vaultto prevent the broadexcept ValueErrorfrom swallowing it (sinceNotteBaseErrorextendsValueError). Four new tests cover the error path, happy path, asymmetric username behaviour, and session-level propagation.Written by Mendral for commit ef6c490.