Conversation
The movement item-read rule compared createdBy to auth.token.email. Guest and kiosk tokens carry no email claim, and guest-created movements carry no createdBy, so null === null let any guest or kiosk session read ownerless movement records directly by key. Require a non-null auth.token.email before the owner comparison in the generated item-read rule (personal-access projects only; shared projects are unaffected). Add rule tests covering direct-key reads of ownerless and owned records by a guest.
Database security rules checked administrator status with .exists() on the /admins/<uid> node, which is satisfied by any stored value (including false, 0 or an empty string). Align the rules with the API layer, which already requires the value to equal true, so only an explicit true grants administrator access. Applies the change to the generated rule constant and to every static occurrence in the rule template. Add rule tests covering a non-true admin record (denied) alongside a genuine admin (allowed).
Workflows referenced actions by mutable refs (branch or major-version tag), so the code executed by a run could change without review. Pin every action to a full commit SHA, keeping a version comment for readability and future updates. Each action is pinned to the commit its current ref already resolves to, so runner behaviour is unchanged: actions/checkout -> v4.4.0 actions/setup-node -> v4.4.0 anthropics/claude-code-action -> v1 w9jds/setup-firebase -> main tip (no release tag published)
The API auth middleware verified ID tokens without the revocation check, so a token stayed valid until its natural expiry even after the session was revoked or the account disabled. Pass checkRevoked=true to verifyIdToken, matching the WebAuthn account-management path; the existing catch already returns 401 for the resulting error. Update the middleware unit test for the new call signature and add a case asserting a revoked token yields 401.
On a save failure the client logged the entire movement object to the browser console, including name, email, phone, remarks and member number. On a shared or kiosk device that PII could persist in the console for a later user. Log only non-identifying context (collection path and record key), matching the payment-method save handler.
The public status page and the start-page banner read the /status node directly from the database, which required a world-readable rule and exposed the full status history (including author name and email) to unauthenticated callers. Serve both from the existing public status endpoint instead, polling for updates, and restrict the raw /status database read to admins (the only remaining direct reader is the admin settings view). The endpoint's output is unchanged, so external consumers are unaffected.
The settlement trigger marked an arrival paid whenever a card payment flipped to success, without checking the paid amount. Compare the paid amount against the arrival's recorded fee (feeTotalGross) and refuse to complete on a mismatch or when no fee is recorded, keeping the existing not-already-completed guard. Note: the recorded fee is still client-supplied; server-computed fees are tracked as follow-up. This closes the amount-mismatch settlement path. Adds tests for matching, mismatching and missing-fee cases.
User-controlled fields (registration, name, remarks, location, etc.) were written to CSV exports unescaped, so a cell beginning with =, +, -, @ (or tab/CR) would be executed as a formula when an admin opens the report in a spreadsheet. Prefix such cells with a single quote so they render as literal text. Applied in the shared CSV writer (covers the movement report) and in the manually built landings report.
Report grouping indexed plain objects with user-controlled strings (invoice recipient name, aircraft registration). A value such as '__proto__' or 'constructor' then interacted with the object prototype instead of being an ordinary key, corrupting the grouping or throwing and breaking report generation. Build these accumulators with Object.create(null) so untrusted keys are always plain data.
The customs completion URL stored on a movement was validated only as a string, so an arbitrary (e.g. phishing) URL could be stored and later opened from the "Open customs" action. Constrain it in the database rules to begin with the configured customs baseUrl (falling back to the previous behaviour when customs is not configured), which rejects the write at the source. As defense in depth, the client now opens only https completion URLs.
Hosting sent only cache headers. Add X-Frame-Options: DENY and a Content-Security-Policy with frame-ancestors 'none' (clickjacking), X-Content-Type-Options: nosniff, a strict Referrer-Policy, and a Permissions-Policy disabling device APIs the app does not use. The CSP is intentionally limited to frame-ancestors so it cannot break resource loading; a full content policy is a separate, larger change.
The user directory is not maintained through the app: the server-side sync endpoint (POST /api/users/import) and the admin CSV user import were unused. Remove both, along with their now-dead supporting code. Server: drop the /users/import route, the syncUsers module, the Basic-auth middleware (its only consumer) and the member-management route gating. Client: drop the imports module, the User import admin tab/form, and the importUsers/importCsv/parseCsv helpers. This also removes the unawaited-deletes bug in the sync path and the Basic-auth-protected destructive route entirely, rather than fixing them. The API_SERVICEUSER credentials and generated member-management flag written by the deploy workflow are now unused and can be removed separately.
airportName and themeColor come from the public sign-in request body and were interpolated into the email HTML unescaped, allowing markup or CSS to be injected into a message sent from the trusted sender. Escape airportName for the HTML output and validate themeColor against a hex or CSS-keyword format (falling back to a default otherwise). Plain-text output keeps the raw name.
The scheduled aerodrome and aircraft-list jobs delete every existing entry that is absent from the fetched feed. An empty, truncated or tampered upstream feed therefore wipes the whole table in one run; the only prior check (updates non-empty) does not prevent mass deletion. Add three fail-closed guards to both jobs: - skip the run when the imported feed yields no valid entries - refuse runs that would delete more than 10% of a populated table (small tables stay exempt so seeding and normal churn still work) - ignore malformed entries missing key fields (icao/name, registration) Extend both specs with empty-feed, excessive-deletion and malformed entry cases.
The public authentication-options endpoint returned real passkey credentials for enrolled emails and an empty allowCredentials list for unknown ones, so its response disclosed whether any given address is a registered user with a passkey (an account enumeration oracle) despite a comment claiming the opposite. Substitute deterministic, secret-keyed decoy credentials when an email has no real passkeys, so enrolled and non-enrolled responses look alike. Decoys are keyed by WEBAUTHN_DECOY_SECRET (per-instance random fallback) to stay unpredictable, and stable per email so repeated probes do not reveal the difference. Real credentials are returned unchanged, so genuine logins are unaffected. Correct the misleading non-disclosure comment and add helper and endpoint tests for the decoy behaviour.
Shared-device access (kiosk display, guest login) uses a fixed Firebase uid and a shared access token. Four weaknesses are addressed: - Kiosk link carried the token in the query string, leaking the shared secret into server logs and the Referer header. Move it into the URL fragment (like the guest link) and read it from there, with a query-string fallback for existing links. - After the token is exchanged, scrub it from the address bar via history.replaceState so it no longer lingers in history or screenshots. - Shared sessions used persistent (local) storage; use session persistence so they end with the browser session. - unauth() did not return the signOut promise, so logout could redirect before sign-out completed; return and await it. Server side: - Drop the unused, unenforced ip custom claim from minted tokens. - Revoke refresh tokens for the kiosk/guest uid whenever the stored access token changes, so rotating the token actually terminates existing sessions (on next refresh, <=1h). Add tests for the new URL parsing/scrubbing, persistence selection, saga behaviour, the token dispatcher, and the revocation triggers.
The /users node held pilot PII (name, email, phone, member number) and was readable by any authenticated session, including guest and kiosk; the contact masking was UI-only. The collection is effectively unused: its only writer (the CSV import) was already removed, it is empty in every project's database, and only one project ever displayed the member field. Remove the node and everything that touched it rather than patch a read rule on a dead PII store: - delete the /users node from the database rules (access now denied by default, so any residual data is unreadable) - keep the member-number field on member-management projects as a plain input instead of the /users-backed autocomplete - delete the users module, the user dropdown container and component, and the renderUserDropdown helpers - drop the auth login name lookup that queried /users (the resolved name was never used and was always empty) Update the affected specs.
The sign-in code verifier capped guesses per code at five, but counted attempts with a read-snapshot-then-update. Concurrent guesses all read the same attempt count and each wrote count + 1, so N parallel wrong guesses advanced the counter by one instead of N. The cap never bit and the six-digit code was brute-forceable within its lifetime. Replace the snapshot-then-update with a per-code-node transaction (the same atomic-claim pattern used for WebAuthn challenges): wrong guesses increment atomically so concurrent attempts are each counted and the cap holds, and a correct code is consumed inside the transaction so it cannot be used twice. Rejections keep the existing generic message. Rework the spec to the transaction model and add cap-enforcement cases.
The prepopulated-forms endpoint was guarded only by fbAuth and forwarded
the raw request body to the customs declaration app using the trusted
integration token. The whole payload was built on the client, so any
authenticated user (including guest/kiosk) could submit arbitrary
declarations under the privileged identity.
Build the payload on the server from the referenced movement instead:
- the client sends only { movementType, movementKey }
- the server loads the movement and aerodrome and constructs the payload
from stored data; externalId is the movement key and aerodromeId comes
from server config, never the client
- strict input validation; unknown movement returns 404
- restrict the route to non-shared sessions (reject guest/kiosk)
- audit-log the caller and movement
Move the payload builder and its helpers from the client saga to
functions/api/customs/buildCustomsPayload.js. Add specs for the builder
and the new middleware; update the client saga spec.
The server-side customs payload builder read movement.date and movement.time, but the stored movement record has neither: it keeps a combined ISO-UTC dateTime plus duration. The client only exposed date/time because it splits dateTime into Europe/Zurich local parts when loading. Server-side those reads were undefined, so the customs form received date and arrivalTime as "Invalid date" and dropped departureTime entirely. Derive the local date and time from the movement's dateTime using Intl (full-ICU, DST-aware), so no timezone dependency is needed, then format the date and compute the arrival time as before. Update the spec to use the real stored shape (dateTime + duration) and assert the derived values.
Authenticated users can write to several collections. Unknown keys were already rejected and numbers/dates/enums were type-bounded, but the free-text string fields had no upper length, so a caller could store multi-MB values and amplify storage/bandwidth cost. Add a max-length validation to every string field on the non-admin writable nodes: departures, arrivals, messages, profiles, and card-payments (also non-admin creatable in the pending state). Caps are generous so legitimate input is never rejected. This bounds size only and does not change payment authorization or any other constraint; the admin-only status node is left untouched. Note: location is free-text (a place name when no ICAO code exists, e.g. "HAGENBUCH"), so it is capped at 100 rather than a short code length.
The public authentication-options endpoint writes a challenge record on every request with no throughput cap, and the cleanup job scanned the whole challenge node hourly, so a flood produced unbounded write, storage and cleanup cost. - Cap concurrency with maxInstances on the challenge-writing option endpoints, giving a global throughput ceiling that bounds worst-case cost without a per-IP/NAT trade-off. - Make cleanup read only expired records via an indexed expiry range query instead of scanning the entire node; index expiry in the rules. App Check is the durable anti-abuse control and is tracked as a follow-up (staged rollout). Per-IP limiting is intentionally avoided (NAT-unsafe on a login endpoint, and adds a write per request).
Run npm audit fix (non-breaking only) to clear the safely-resolvable advisories in the functions dependency tree. Lockfile-only change, no package.json edits; the full functions test suite still passes. The remaining advisories require major upgrades (nodemailer 8 to 9 and firebase-admin to 14 for the uuid subtree) and are left for a separate, tested upgrade rather than forced in. The reachable dependency (nodemailer) is only vulnerable via the raw message option, which the sign-in email flow does not use.
First step of moving fee computation server-side so fees become authoritative rather than client-supplied (payment-integrity program). Ports the lspl fee strategy and the shared orchestration (origin classification, VAT, five-cent rounding) into functions/fees, with parity tests copied from the client lspl spec so the server always computes the same fee the client displays. Engine only: not yet wired to a trigger or the rules. Only lspl is ported (the pilot project); other projects are unaffected. An unknown registration classifies as OTHER (no discount), so a faked registration can only ever cost more.
Add an onValueWritten trigger on /arrivals that recomputes the landing fee with the server-side fee engine and writes the authoritative fee fields back to the record. The trigger is gated per project by /settings/landingFeesStrategy: absent means the project is not on server-owned fees and the handler is a no-op, so it lands inert everywhere until a project opts in. Only strategies ported into functions/fees are honoured; an unrecognised one fails closed and writes nothing. For a known registration the MTOW and category come from the aircraft registry; otherwise the submitted values are used and the record is flagged aircraftDataSource=manual. The pilot's own declared mtow/aircraftCategory are never overwritten. A change guard avoids re-triggering on the handler's own write.
Add an opt-in `scopeCardPaymentsToOwner` per-project flag. When set, the card-payment rules only let the authenticated creator (or an admin) read a payment and cancel their own pending one, and a new payment must carry the creator's own uid as `owner`. Projects that do not opt in keep the previous auth-only rule byte-for-byte, so their behaviour is unchanged. The client now stamps `owner` (the caller's uid) on the payment it creates; the field is optional and permitted for every project via a new `owner` validation rule, but only enforced where the flag is on. Enable the flag for lspl. The payment webhook writes via the Admin SDK and bypasses these rules, so its flow is unaffected.
The movement lock decided whether a record fell in the closed period from the client-supplied numeric `negativeTimestamp`, which nothing tied to the ISO `dateTime` the record actually stores and reports display. A direct database writer could pass the lock with a recent `negativeTimestamp` while backdating `dateTime` into a frozen period. Rules cannot convert the numeric field to a date, so move the lock onto `dateTime` itself. A new trigger derives `settings/lockDateIso` (the lock threshold plus the existing one-day grace, as an ISO-UTC string) from `settings/lockDate` whenever it changes; fixed-width ISO-UTC strings sort chronologically, matching the `dateTime` regex, so the rule comparison is exact. The derived field is server-written and client-immutable (settings default-deny), so it cannot be forged. Gated by an opt-in `lockOnDateTime` flag: projects that do not opt in generate the previous numeric rule byte-for-byte, and the trigger only writes the mirror where `/settings/lockOnDateTime` is set. While the mirror is briefly absent the rule falls back to the numeric test, so it is never weaker than before and never blocks writes. Enabled for lspl.
The static username/password login had no rate limiting, so a shared credential could be guessed with unlimited online attempts. Add a per-IP failed-attempt counter: after 10 failures from one IP within 15 minutes, further attempts from that IP are rejected until the window elapses; a successful login clears the counter, so normal use never accumulates. A blocked attempt returns the same result as a wrong password, revealing nothing. The counter reuses the existing windowed-limiter pattern and is stored under the server-only /staticAuthRateLimits node; the hourly cleanup job prunes elapsed windows so it cannot grow unbounded. Only projects that configure static credentials ever touch it. This is a best-effort deterrent (the client IP comes from X-Forwarded-For); the durable defenses remain a strong shared credential and moving this login to the standard mechanisms. Also make requestHelper.getIp tolerate a missing headers/connection.
Two remaining server-side controls for arrival billing integrity. Rules now restrict paymentMethod.method to the known set (card, checkout, cash, card_external, twint_external, invoice) instead of any non-empty string. On the trusted arrival write path, authorize the declared invoice recipient: a recipient is only kept when the arrival author's authenticated email is listed on it in settings/invoiceRecipients; otherwise it is cleared. The recipient list is an array the rules cannot search, so this is enforced server-side, gated by the same landing-fee-strategy switch as the fee recompute.
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
2 participants
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.
No description provided.