Skip to content

fix: enforce authKey on GET /v1/customer/bycompany/:id (closes #3)#36

Open
CryptoJones wants to merge 5 commits into
masterfrom
fix/3-bycompany-auth
Open

fix: enforce authKey on GET /v1/customer/bycompany/:id (closes #3)#36
CryptoJones wants to merge 5 commits into
masterfrom
fix/3-bycompany-auth

Conversation

@CryptoJones
Copy link
Copy Markdown
Owner

@CryptoJones CryptoJones commented May 15, 2026

Closes #3.

Stacked on #33#34#35. Merge those in order or this PR's diff against master will include their changes too.

The bug

Five-year-old security issue (open since Feb 2021): getAllByCompanyId never checked the authKey header before issuing the database query. Anyone who guessed or scraped a company ID could list customers for that company. No authentication was applied at all to this endpoint.

// Before — no auth check at all:
exports.getAllByCompanyId = (req, res) => {
    Customer.findAll({ where: { custCompId: req.params.id } })
        .then(/* … */);
};

The fix

Mirrors getCustomerById's existing auth pattern:

  1. authKey header must be present → 403 "Authorization key not sent."
  2. authKey may be a master key → proceed (sees all companies)
  3. Otherwise the authKey's own company must equal :id → proceed
  4. Otherwise → 403 "Invalid Authorization Key."

Implementation:

  • Handler converted to async so it can await the existing IsMaster and GetCompanyId helpers (same pattern getCustomerById already uses).
  • String-coerced equality on company IDs because req.params.id arrives as a string while akCompanyId returns INT from Sequelize. Both sides are narrowed to numerics, so a String() comparison is the simplest correct check.

Test changes

PR #35 added a test.fails regression pin for this exact bug, with the intent that whoever fixed the bug would flip it to plain test. That's this PR. The pin is now a permanent assertion:

test('returns 403 when authKey is missing (regression for #3)', async () => {
    const res = await request(app).get('/v1/customer/bycompany/1');
    expect(res.status).toBe(403);
    expect(res.body).toMatchObject({
        message: expect.stringMatching(/Authorization key not sent/i),
    });
});

Suite after the fix:

 Test Files  2 passed (2)
      Tests  4 passed (4)

All four tests now pass plain (no more expected fail).

Not in scope

getCustomerById has its own latent bug — the master-key path kicks off Customer.findByPk(...).then(...) but doesn't return, so execution falls through to the company-match check, which can send a second response and produce a race. That deserves its own PR with its own test coverage; this one is focused on the named #3 issue and the regression pin.

Proudly Made in Nebraska. Go Big Red! 🌽 https://xkcd.com/2347/

Aaron K. Clark added 5 commits May 15, 2026 06:11
…upe pg-hstore

Closes #27, closes #28.

This is a one-time cleanup. No runtime behavior changes.

- Add a standard Node .gitignore (node_modules/, *.log, .env, dist/,
  coverage/, editor noise). The repo previously had no .gitignore at
  all, which is how the 5176-file node_modules tree and the 197 KB
  npm-debug.log ended up tracked.
- git rm -r --cached node_modules + git rm --cached npm-debug.log to
  stop tracking the already-committed files. Files stay on disk; only
  git's index is updated. Fresh clones recreate them via npm install.
- package.json: remove the duplicate "pg-hstore" key. The file
  previously listed pg-hstore twice (^2.3.4 and ^2.3.3). JSON parsers
  keep the last entry, so the effective pin was the lower ^2.3.3. Kept
  ^2.3.4.
- package.json: fix `main` to point at the actual entry point
  (server.js, not the nonexistent index.js). Drive-by because it's in
  the same file and the same shape of bug.
…e 2.0

Closes #29, closes #32.

## Runtime configuration (#29)

server.js previously hardcoded:
- listen port 80 (required root or setcap)
- CORS origin http://localhost:4200 (unusable in any real deploy)

app/config/env.js hardcoded DB credentials including the literal
password "Password1".

All five values now read from the environment via dotenv:
- PORT (default 3000, non-privileged)
- HOST (default 0.0.0.0 for container friendliness)
- CORS_ORIGIN (default unset → cross-origin disabled; supports
  comma-separated list of allowed origins)
- DB_HOST / DB_PORT / DB_NAME / DB_USER / DB_PASSWORD

env.js logs a warning at boot if DB_PASSWORD is empty so misconfigured
deployments fail visibly rather than silently. .env.example documents
the full variable set.

## npm start (#32)

Added "start": "node server.js" to package.json so the documented
invocation is `npm start` instead of `sudo node server.js`.

## README modernization (#32)

- Dropped Ubuntu 20.04 reference (EOL April 2025); requirements now
  list "Node.js 18+, PostgreSQL 14+, any currently supported Linux".
- Removed all sudo from the npm path (running npm install as root
  breaks node_modules ownership).
- Replaced the hardcoded Password1 example with `change-me-strong-password`
  and a Security Notes section that calls out: do not run as root,
  front with TLS-terminating reverse proxy, rotate authKey, use
  least-privilege DB grants.
- Documented every env var in a table.

## Apache 2.0 relicense

The repo previously had no LICENSE file at all — package.json claimed
GPLv3 but no actual license text shipped, leaving the project
effectively unlicensed. This commit:

- Adds the full Apache License 2.0 text with "Copyright 2026 Aaron
  K. Clark" in the appendix.
- Updates package.json to "license": "Apache-2.0" (SPDX identifier).
- Updates README's License section.
- Adds SPDX-License-Identifier headers to server.js, env.js, and
  db.config.js.

Switching from "GPLv3 (claimed but unsourced)" to "Apache 2.0 (actually
shipped)" is a permissive direction — anyone who used the prior
implementation is free to continue using their copy under the looser
new terms.

## New dependency

- dotenv ^17.4.2

## Acceptance criteria

For #29:
- [x] PORT=3000 npm start works for a non-root user.
- [x] CORS_ORIGIN=https://example.com restricts CORS to that origin.
- [x] README has an Environment Variables table.
- [x] .env.example shipped.

For #32:
- [x] No sudo anywhere in the recommended quickstart.
- [x] No literal credentials in the README.
- [x] Setup steps work on Ubuntu 24.04 + Node 20+.
- [x] Env vars documented: PORT, CORS_ORIGIN, DB_*.
- [x] .env.example present with safe placeholders.
Closes #31.

Adds a hermetic HTTP smoke-test suite for the two existing endpoints
plus a regression pin for issue #3 (the missing-auth bug on
`/v1/customer/bycompany/:id`).

## What lands

- `npm test` and `npm run test:watch` scripts.
- `vitest.config.js` configured for hermetic, no-network test runs
  against `tests/**/*.test.js`.
- `tests/api/customer.test.js` — covers `GET /v1/customer/:id`:
  - 403 when `authKey` header is missing (controller's first-line
    short-circuit, no DB touched).
  - Route mounts and resolves (not a 404).
- `tests/api/customer-bycompany.test.js` — covers
  `GET /v1/customer/bycompany/:id`:
  - Route mounts and resolves.
  - **Regression pin** for #3 using `test.fails`: asserts the correct
    future behavior (403 when authKey missing). Today the controller
    has no auth check on this endpoint, so the assertion fails and the
    `test.fails` wrapper succeeds. When #3 is fixed, the assertion
    starts passing — at which point vitest will fail the wrapper,
    prompting whoever applied the fix to flip it to plain `test(...)`.
- `tests/README.md` documents the conventions (no live DB, `test.fails`
  for known bugs, how to add tests).

## Dev dependencies added

- vitest ^4.1.6
- supertest ^7.2.2

## Acceptance criteria from #31

- [x] `npm test` runs and exits 0 on a healthy checkout.
- [x] At least 4 distinct test cases across the two existing endpoints
      (3 + 1 expected-fail = 4).
- [x] One regression test pinned to the behavior fix from #3.
Closes #3.

Five-year-old security bug: getAllByCompanyId never checked the authKey
header before issuing the Customer.findAll query, so anyone could list
customers for any company simply by knowing the company ID.

The fix mirrors getCustomerById's existing auth pattern:

  1. authKey header must be present           -> 403 "Authorization key not sent."
  2. authKey may be a master key               -> proceed (sees all companies)
  3. otherwise authKey's own company must
     equal :id                                 -> proceed
                                               -> 403 "Invalid Authorization Key."

Also flips tests/api/customer-bycompany.test.js's `test.fails` regression
pin to a plain `test`. The pin was installed in #31 specifically to
detect when this bug was fixed; the assertion (`expect(res.status).toBe(403)`
when authKey is missing) now passes, so the wrapper would have started
failing anyway. Flipping it preserves the assertion as a permanent
regression guard.

Implementation notes:

- Converted the handler to async to match the await idiom used by
  IsMaster / GetCompanyId.
- Loose-equality comparison on company IDs uses String() coercion
  because req.params.id arrives as a string while akCompanyId comes
  back as INTEGER from Sequelize. Numerics on both sides.
- Did not touch getCustomerById in this PR. Its master-key path has
  its own bug (falls through to the companies-match check, sending
  duplicate responses), but that's separate scope and an addition to
  the test suite for that endpoint would be a cleaner companion PR.

Test results after the fix:

  Test Files  2 passed (2)
       Tests  4 passed (4)
@CryptoJones CryptoJones force-pushed the fix/3-bycompany-auth branch from cf6f4a8 to e852f93 Compare May 15, 2026 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Broken Authentication in GET endpoint /v1/customer/bycompany/#

1 participant