Repository navigation
fix: validate document keys before they reach request URLs - #3
Merged
Merged
Conversation
Keys are placed into pyArango document URLs unencoded. `_make_key` now checks the joined key against ArangoDB's document-key alphabet (minus `%`, which the server would percent-decode), rejects `.`/`..` and keys over 254 bytes, and raises KeyError otherwise. New public helper: `validate_document_key`. CI: live-server tests and doctests are skipped when no ArangoDB server is reachable (they previously errored at collection), and docsrc/ is excluded from test collection. Closes #2 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Closes #2
What changes
validate_document_key(key);ArangoDbPersister._make_keyruns every joined key through it. A key must match ArangoDB's document-key alphabet (A-Za-z0-9_-:.@()+,=;$!*'), be 1–254 bytes, and not be.or..; otherwiseKeyErroris raised before anything reaches the backend (reads, deletes and writes), sos[k]/s.get(k, d)behave like a missing key.%is excluded from the alphabet even though ArangoDB allows it: pyArango does not encode keys in URLs and the server percent-decodes the path, so an encoded separator would bring back the same problem. Such keys did not round-trip correctly before either.arangodol/tests/test_keys.py(no server needed).CI
Master CI was red:
test_arangodb.pyand the class doctests connect to a local ArangoDB server at import/collection time. They are now skipped when no server is reachable (arangodol/tests/_live_server.py, rootconftest.py), anddocsrc/is excluded from test collection.wads ci-localpasses on 3.10 and 3.12 (live-server tests skipped; they were not run).Review
Independent refute-review by a sub-agent: no blockers. Applied:
__setitem__validates the key before any backend call, clearer comment on why%is excluded, more rejection cases and a test that invalid keys never reach the backend. Noted, not changed:db_name/collection_nameare also unescaped in URLs (configuration, not user data); class doctests still import the oldpy2storepath and are skipped without a live server.Compatibility
Valid ArangoDB keys are unaffected. No fleet dependents.
🤖 Generated with Claude Code