Fix multiple security vulnerabilities (auth bypass, path traversal, XSS, weak defaults)#7
Open
yourfavDev wants to merge 1 commit into
Open
Fix multiple security vulnerabilities (auth bypass, path traversal, XSS, weak defaults)#7yourfavDev wants to merge 1 commit into
yourfavDev wants to merge 1 commit into
Conversation
Unauthenticated data exfiltration
- GET /{table}/store, /{table}/keys, POST /{table}/keys and the SSE
/{table}/subscribe/{key} endpoint required no authentication, so any
caller could dump every record in a table — including the auth table
with all users' hashed passwords. They now require a valid JWT and only
return rows owned by the caller.
Insecure secret fallbacks
- ADMIN_PASSWORD silently defaulted to "admin123", CLUSTER_SECRET to
"default_secret", and admin JWT signing to "default_jwt_secret". A
misconfigured node therefore accepted X-Internal-Request: default_secret
on the get_value / delete_value / put_value_internal endpoints, granting
full read/write/delete to anyone on the network. main.rs now refuses to
start unless JWT_SECRET, CLUSTER_SECRET and ADMIN_PASSWORD are set,
non-empty, longer than 16 bytes and not in the known-placeholder list.
admin_login no longer falls back to a hard-coded password and uses a
constant-time comparison.
Path traversal via {table}/{key}
- Table and key names from URL paths flow into Path::join in
persistance.rs. Names containing "/", "\\", "\\0", "." or ".." (or
exceeding 255 bytes) are now rejected at every handler that touches the
store before they can reach the persistence layer.
Plaintext cluster traffic when DYNA_MODE=https
- engine.rs / admin.rs / authentication.rs hard-coded http:// when
building internal replication URLs, leaking the cluster secret and
every replicated payload even when TLS was configured. All internal
URL construction now goes through network::broadcaster::build_url so
the scheme follows DYNA_MODE.
Race condition + ownership forgery in user registration
- The check-then-insert in authentication::access could race two
concurrent registrations of the same username. The internal replication
branch also trusted an attacker-controlled "User" header as the record
owner. Registration is now atomic via DashMap's entry API and the owner
is always the path-derived username. Passwords are salted with the
username before hashing (note: existing accounts must re-register).
Stored XSS in admin dashboard
- admin.html rendered record.owner and vector_clock keys with innerHTML,
so any value an attacker could PUT would execute JS in the admin's
browser. Replaced with safe DOM construction (textContent).
Committed weak credentials
- Removed the tracked .env containing JWT_SECRET=123 / ADMIN_PASSWORD=
admin123 / CLUSTER_SECRET=please_change, added .env to .gitignore, and
shipped a .env.example with guidance.
https://claude.ai/code/session_01NYckTQH6APLpfGKVwwcQVD
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
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
Audit of the codebase surfaced several high-impact issues. Each is addressed in this PR.
1. Unauthenticated data exfiltration (Critical)
GET /{table}/store,GET /{table}/keys,POST /{table}/keysand the SSEGET /{table}/subscribe/{key}endpoint required no authentication. Any anonymous client could:and walk away with every user's SHA256-hashed password for offline cracking, plus the full contents of every table.
Fix: all four endpoints now require a valid JWT and only return rows whose
owner == subclaim.2. Insecure secret fallbacks (Critical)
ADMIN_PASSWORDsilently defaulted to"admin123".CLUSTER_SECRETdefaulted to"default_secret"everywhere.get_jwt_secret()that defaulted to"default_jwt_secret".Combined with the existing
X-Internal-Request: <CLUSTER_SECRET>bypass inget_value,put_value_internalanddelete_value, a node that booted without configured secrets would happily read/write/delete any record for any caller using the default header.Fix:
main.rsnow refuses to boot unlessJWT_SECRET,CLUSTER_SECRETandADMIN_PASSWORDare all set, ≥16 bytes long, and not in the known-placeholder list ("","default_secret","please_change","admin123", ...).admin_loginno longer falls back to a hard-coded password and now uses a constant-time comparison. Admin signing/verification uses the strictengine::get_jwt_secret.3. Path traversal via
{table}/{key}(High)Table names from URL paths are joined into filesystem paths in
persistance.rs(Path::new(state.base_dir).join(&t_name)). A request likewould write outside
./data.Fix: new
is_safe_name()helper rejects empty names,.,.., names containing/,\, or\0, and names longer than 255 bytes. Applied to every handler (get_value,put_value,patch_value,delete_value,put_value_internal,get_table_store,get_all_keys,get_multiple_keys,subscribe_to_key, admin handlers,authentication::access).4. Plaintext cluster traffic when
DYNA_MODE=https(High)engine.rs,admin.rsandauthentication.rshard-codedformat!("http://{}/...")for all internal replication URLs even when TLS was configured, leaking the cluster secret and every replicated value over the wire.Fix:
network::broadcaster::build_urlwas already TLS-aware; it is nowpuband used by every internal call site.5. Race condition + ownership forgery in user registration (High)
authentication::accessdid a non-atomicauth_table.get(&username)thenauth_table.insert(...), so two concurrent registrations of the same username could race past each other. The internal replication branch also trusted a caller-controlledUser:header as the record owner.Fix: registration is now atomic via DashMap's
entry().and_modify(...).or_insert_with(...). The owner is always the path-derivedusername; the spoofableUser:header is gone. Passwords are now salted with the username before SHA256 to defeat rainbow tables.6. Stored XSS in admin dashboard (Moderate)
admin.htmlrenderedrecord.ownerandvector_clockkeys withinnerHTML. Any user who PUT a record with an owner containing<img src=x onerror=...>could pop JS in the admin's browser the next time they viewed the record.Fix: replaced the template-literal
innerHTMLblock with safe DOM construction (textContent+createElement+appendChild).7. Committed weak credentials (Moderate)
.envwas tracked in the repo withJWT_SECRET=123,ADMIN_PASSWORD=admin123,CLUSTER_SECRET=please_change.Fix: removed
.env, added it to.gitignore, and added a.env.examplewith guidance. Anyone who has ever deployed using those values should rotate them immediately — they are now in the public git history.Test plan
cargo buildsucceeds with no warnings.fatal: ... refusing to startmessage.JWT_SECRET=123→ process exits (too short).CLUSTER_SECRET=default_secret→ process exits (forbidden value).DYNA_MODE.GET /auth/storewithout a token → 401.GET /default/storewith user A's token → only returns rows whereowner == A.PUT /..%2Fetc/key/foo→ 400Invalid table or key name, no file written outside./data.owner = "<img src=x onerror=alert(1)>"; reopen it in the dashboard → renders as text, no alert.https://claude.ai/code/session_01NYckTQH6APLpfGKVwwcQVD
Generated by Claude Code