Admin logs: no full client IP, no plaintext address, and the :8090 gateway written down - #374
Merged
Merged
Conversation
/privacy says an IP is processed transiently for security and abuse prevention and is never logged for analytics or profiling, and /security says nginx access logging is off. Three admin routes wrote the full client IP to stdout, which is the container log and is not transient: both [signup] lines and [totp-reset-req] rate limited. The second [signup] line carried the full e-mail address next to it. admin/lib/log-redact.js is the code side of that claim now. maskIpForLog truncates to /24 and /48 and writes the host part as an x, so a truncated address cannot be read as a real one and a full IPv4 or IPv6 in a log line is something a regex can find. maskEmailForLog keeps one character and the domain, which is what the file already did for four other lines. logRedacted re-scrubs the finished line, so a new statement on these paths cannot leak by omission. admin/test/log-redact.test.js lifts the three statements out of admin/server.js, runs them against a captured console and asserts no full IPv4, no full IPv6 and no plaintext address in the output. It also sweeps the file for any log statement that interpolates a raw ip, norm or email. Five sabotages, five failures: each call site reverted, the raw ip interpolated back in, and each of the three helpers neutered. The :8090 nginx block was checked while looking for the same claim in the web server. It is not dead: the port appears nowhere else in the repo, but location /dicom/ proxies into it and docs/dicom-guide.md sells that path as the Enterprise STOW-RS gateway. So it stays, with a note above it saying what it costs: the catch-all forwards to a Fly host outside the EEA, resolver 8.8.8.8 puts Google Public DNS in the path of every request, and neither is on the sub-processor list in /dpa. Those are operator decisions. access_log off is not, and was missing. docs/site-claims.md row 9 now covers both halves of the IP-logging claim, and the fly.dev route is filed as an open contradiction.
…aims Four things the review was right about. The scrubber's lookbehind excluded ":", so "ip:203.0.113.42" and "ipv6:2001:db8:85a3::1" went through untouched. A colon in front of an address is the ordinary shape of a log line, not a reason to skip it. The lookbehind is [0-9A-Za-z.] now, which still leaves std::vector, ns::method and a bare "::" in prose alone, and the "::" alternative requires at least one hextet after it so prose cannot match. Ten separator shapes are in the test, and the four-part version number the scrubber over-masks is pinned as an accepted trade rather than fixed. The comment no longer calls itself a last line of defence: it is a backstop, the call sites carry the claim, and the gaps are written down. The :8090 access_log off was not pinned. The block filter in tests/site-claims.test.mjs matches a docroot or a 127.0.0.1 upstream and :8090 has neither, and the doesNotMatch above it only fires on a log pointed at a file, so deleting the line left every suite green. The filter takes the block now, there is an assert that names it, and the comment claiming the block only fronts the Outlook add-in is corrected: it answers for location /dicom/. Deleting the line fails; pointing it at a file fails. The justification in the conf was wrong. set_real_ip_from and real_ip_header are in the :8080 block, not this one, and location /dicom/ forwards no X-Real-IP, so $remote_addr there is 127.0.0.1 and the line never carried a client IP. It carried the request line, URI, timing, status, referrer and user-agent. Same correction for what crosses to the Fly host: the request line, the client's own headers and the body, not the IP. And the access_log off does not reach the server. Phase 5c of deploy/deploy-3.1.sh (around line 1226) edits the live confs by anchor and never copies this file. Claim limited to the repo conf in the note and in site-claims row 102; the 5c edit waits for #376, which reshapes 5c. The unused eslint-disable in the test is gone.
Apolloccrypt
force-pushed
the
fix/no-full-ip-in-logs
branch
from
September 3, 2026 00:10
963dcd7 to
a213e15
Compare
Apolloccrypt
added a commit
that referenced
this pull request
Sep 3, 2026
… open CI was red on this branch for a reason that had nothing to do with the runner. The block asserted that admin/server.js writes three log lines with a full client IP. It did when the block was written. #374 landed first and routed all three through admin/lib/log-redact.js, so on the merge commit the count is zero and the assertion was right to fail: /privacy was describing a system that no longer exists. The page now says what the code does. An address is truncated to its network before it is written and an email keeps one character and its domain, so the container log holds neither in full. The section is renamed accordingly and the admin entry stands as contrast rather than as the finding. The block keeps only what this pull request actually adds: the edge access log, which no page described, pinned to scripts/access-log-visitors.mjs. The claim about the admin lines belongs to row 9 and admin/test/log-redact.test.js, which pin it at the source, and a second weaker copy here would only rot again. What remains of that half is a guard: /privacy may not go back to describing raw lines, and the masking it does describe has to still exist. Five sabotages red in both directions: dropping the section, restoring "only transiently", pointing the reader at another file, renaming maskIpForLog, and putting the raw-lines sentence back.
Apolloccrypt
added a commit
that referenced
this pull request
Sep 3, 2026
… open CI was red on this branch for a reason that had nothing to do with the runner. The block asserted that admin/server.js writes three log lines with a full client IP. It did when the block was written. #374 landed first and routed all three through admin/lib/log-redact.js, so on the merge commit the count is zero and the assertion was right to fail: /privacy was describing a system that no longer exists. The page now says what the code does. An address is truncated to its network before it is written and an email keeps one character and its domain, so the container log holds neither in full. The section is renamed accordingly and the admin entry stands as contrast rather than as the finding. The block keeps only what this pull request actually adds: the edge access log, which no page described, pinned to scripts/access-log-visitors.mjs. The claim about the admin lines belongs to row 9 and admin/test/log-redact.test.js, which pin it at the source, and a second weaker copy here would only rot again. What remains of that half is a guard: /privacy may not go back to describing raw lines, and the masking it does describe has to still exist. Five sabotages red in both directions: dropping the section, restoring "only transiently", pointing the reader at another file, renaming maskIpForLog, and putting the raw-lines sentence back.
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.
Two findings out of the legal review in #372, both about the same promise: that Paramant does not keep a visitor's IP address.
Review round 1 corrected four things. They are marked [r1] below.
1. Three admin routes wrote a full IP to the container log
/privacy says an IP is "processed only transiently, for security and abuse-prevention" and that Paramant does not "log or store IP addresses for analytics or profiling". /security says nginx access logging is off. The web server holds up. The application did not.
grepoveradmin/andrelay/forreq.ip,x-forwarded-for,remoteAddressand address interpolation in log calls found exactly three offenders, all inadmin/server.js, all writing to stdout, which on this deployment is the container log and is not transient. The relay was already clean: everylog()there runs the value through its ownmaskIp/maskEmail.admin/server.js:767POST /api/user/signup, duplicate branch[signup] duplicate signup attempt: ${emailHash.slice(0,8)} from ${ip}... from 203.0.113.xadmin/server.js:785POST /api/user/signup, new-address branch[signup] pending signup for ${norm} from ${ip}[signup] pending signup for d***@example.com from 203.0.113.xadmin/server.js:1916POST /api/user/auth/request-totp-reset, rate limiter[totp-reset-req] rate limited: emailHash=${emailHash} ip=${ip}... ip=203.0.113.xThe middle one is the worst of the three: it put a full e-mail address and the full IP that submitted it on the same line, and it fires on the ordinary signup path, not on an error.
The form
admin/lib/log-redact.js, new:maskIpForLogtruncates to /24 for IPv4 and /48 for IPv6. Truncation rather than a salted hash because this repo already truncates everywhere else it masks an address (relay/relay.jsmaskIp,admin/lib/audit.js_maskIp,admin/server.jsmaskIpfor the sessions API). No salt to rotate, no key to lose.x, not as0.203.0.113.0is still a complete IPv4 address and a reader cannot tell it from a real one;203.0.113.xis not an address at all, which is what makes "no full IP reaches the log" something a test can assert with a regex. Compressed IPv6 is expanded before the /48 is taken, sofe80::1and2001:db8:85a3::1are not read off their written form. IPv4-mapped IPv6 (::ffff:203.0.113.42) is masked as the v4 address it is.maskEmailForLogkeeps the first character and the full domain, the formadmin/server.jsalready used on four other lines. The domain stays because without it these lines cannot answer which provider is flooding signup, which is the only reason they exist. The localmaskEmailinserver.jsis now this one function, imported, instead of a second copy.logRedactedruns the finished line through a scrubber before it reachesconsole.[r1] The scrubber had a hole and it was the common case. Its lookbehind excluded
:, soip:203.0.113.42andipv6:2001:db8:85a3::1passed through untouched. A colon in front of an address is the ordinary shape of a key/value log line, not a reason to skip it. The lookbehind is[0-9A-Za-z.]now. Ten separator shapes are in the test, including all of the reviewer's.The lookbehind still blocks a word character, and that is load-bearing: it is the only thing keeping
std::vector,ns::methodandfoo::barintact. The::alternative now also requires at least one hextet after it, so a bare::in prose is not an address. The trade is written down in the source rather than left implicit: an address glued straight onto a word (addr2001:db8::1) is skipped, and a four-part version number (1.2.3.4) is over-masked. The over-mask is pinned as an accepted decision, not fixed: over-scrubbing costs a digit, under-scrubbing costs an IP. The comment no longer calls the scrubber a last line of defence. It is a backstop; the masking the claim rests on is at the three call sites, and that is where the test asserts it.maskIpinserver.jsis untouched. It renders theip_maskedfield of the sessions API response and is not a log form; a comment now says so.The test
admin/test/log-redact.test.js, picked up by the existingadmin-unit-testsjob (node --test test/*.test.js).It does not assert against a copy of the log lines. It reads
admin/server.js, lifts the three statements out by their tag, and executes each one against a capturedconsolewith a documentation-range IPv4, a documentation-range IPv6 and a sample address bound toip/norm/email. The captured output is then asserted to contain no full IPv4, no full IPv6, no plaintext address, and none of the three inputs verbatim. It separately sweeps the whole file for anyconsole.*orlogRedactedcall that interpolates a rawip,norm,email,clientIporremoteAddress, so a fourth path cannot reintroduce this by copying an old one.Six sabotages, six failures:
console.logwith${norm}and${ip}logRedactedbut interpolates the raw${ip}maskIpForLogreturns the address unchangedmaskEmailForLogreturns the address unchangedredactbecomes a pass-throughip:203.0.113.42docs/site-claims.mdrow 9 was the nginx half of the claim only. It now carries both halves and names the helper and the test.2. The :8090 block is not dead
grep 8090over docs, compose, scripts, the installers and the deploy scripts returns nothing. Insidedeploy/nginx-paramant-live.confit returns one consumer:location /dicom/in the public server block proxies tohttp://127.0.0.1:8090/, and the:8090catch-all doesproxy_pass https://<fly host>withresolver 8.8.8.8.docs/dicom-guide.mdsells that path as the Enterprise STOW-RS gateway. Deleting the block would turn/dicom/into a 502, so it stays and is documented instead. The comment further up that left this "pending an operator decision" now records the decision.What the note above the block says, because none of it is fixable by a repo edit:
Hostis overridden) and the body, which the sector guide describes as imaging data. The client IP is not among it: nothing on that path setsX-Real-IPorX-Forwarded-For, so Fly sees this machine's connection. The payload is the exposure, not the IP. The earlier version of this bullet said otherwise and was wrong.resolver 8.8.8.8makes Google Public DNS a party to every request on this path. It learns the upstream name and the timing on each resolve. That is a sub-processor in its own right, in a config whose surrounding promise is EU-only, and it is not on the list either. An EU resolver or a static upstream address removes that party.proxy_passover https with noproxy_ssl_verify. The/rp/fly/block was removed for exactly this. That one forwarded credentials and this one does not, but the unverified TLS is still real.access_log off: repo conf only, not the server
The block was the only one in the file without
access_log off, and /security claims logging is off on every block that serves the site. The line is added here.[r1] It does not reach the server, and the PR previously claimed it did.
deploy/deploy-3.1.shphase 5c (around line 1226) edits the live confs by anchor and never copies this file over them, so the server keeps whatever it has. The 5c edit is deliberately not in this change: #376 reshapes 5c and the two would collide. Row 102 ofdocs/site-claims.mdstates the limit and says what still has to happen.[r1] And the reason given for it was wrong.
set_real_ip_fromandreal_ip_headerare in the:8080block at the top of the file, not this one, andlocation /dicom/forwards noX-Real-IP, so$remote_addrin the:8090block is127.0.0.1on every request. The line never carried a client IP. What the default combined format recorded is the request line and URI, the timing, the status, the referrer and the user-agent of each/dicom/call. Worth switching off, and not an IP leak. The conf comment says this now.[r1] The line is pinned
tests/site-claims.test.mjsdid not cover it. Its block filter matchesroot /home/paramantor a127.0.0.1upstream, and:8090has neither (it usesalias, and its catch-all proxies to an external https upstream); thedoesNotMatchabove the loop only fires on anaccess_logpointed at a file, so deleting the line left all 22 assertions green. Reproduced before fixing.The filter takes the block now (third arm on
listen 127.0.0.1:8090), the floor moved from 6 blocks to 7, and there is an explicit assert that names the block so a retired block is not silently "nothing to check". The comment claiming the last block "only fronts the Outlook add-in via an external host" is corrected: its/outlook/*aliases duplicate the:8081block and are served for real fromaddin.paramant.app, while what the block actually does is answer for/dicom/.Sabotage in both directions:
access_log off;deleted from:8090access_log /var/log/nginx/dicom.log;in:8090The
/outlook/*aliases themselves are left alone: that is a separate, provable cleanup.docs/site-claims.mdgains the fly.dev route as an open contradiction, including thatdocs/dicom-guide.mddescribes the gateway as proxying "directly into the Ghost Pipe relay", which the config contradicts.Tests
admin/test/*.test.jsrelay/test/*.test.jstests/*.mjs(node-only)tests/site-claims.test.mjstests/static-sanity.shscripts/check-test-declarations.sheslint@9 .tests/deploy-3.1-dryrun.test.shBoth failures are pre-existing and reproduce identically on
origin/mainat 78f1750 in this environment:relay/test/parasign-envelope-index.test.jsneeds a Redis on 6396 that is not running, andtests/heartbeat-lib.test.mjscannot resolve@noble/post-quantum.Coordination
Rebased on
origin/mainat 78f1750 (#375).deploy/DEPLOY-3.1.mdanddeploy/deploy-3.1.share untouched: nothing is removed from the nginx config, and the phase 5c edit that would carryaccess_log offto the server waits for #376.frontend/privacy.html,frontend/dpa.htmlandfrontend/terms.htmlare untouched, as are all other frontend pages, so #372 and #369 are clear on that side. This PR touchesdeploy/nginx-paramant-live.confat the/dicom/location, the/rp/fly/note and the:8090block;docs/site-claims.mdat existing row 9 and two appended rows in the unpinned tables; andtests/site-claims.test.mjsinside the row-9 test only.