Skip to content

fix(identity-firmographic): crypto.randomUUID ids; distinguish absent vs empty-string fields - #15

Open
mdheller wants to merge 1 commit into
mainfrom
fix/id-firmographic-uuid-and-empty-string
Open

fix(identity-firmographic): crypto.randomUUID ids; distinguish absent vs empty-string fields#15
mdheller wants to merge 1 commit into
mainfrom
fix/id-firmographic-uuid-and-empty-string

Conversation

@mdheller

Copy link
Copy Markdown
Member

Summary

Two small correctness fixes in services/identity-firmographic-alpha-runtime/server.js:

  1. Replace Date.now() + Math.random().slice(2,10) id generation with crypto.randomUUID().
  2. Fix the truthy-check on doc.company_name / doc.person_name so that a payload with an empty-string field is not silently classified as absent.

1. Id-collision surface

Before:

function id() {
  return \`rec_\${Date.now()}_\${Math.random().toString(36).slice(2, 10)}\`;
}

Under concurrent ingest at the same millisecond, the only entropy separating two ids is the 8-hex-char random suffix, i.e. ~2^42 (~4.3B) distinct values. Birthday-collision territory ($\sim\sqrt{2^{42}} \approx 2M$) sits well within the requests-per-day this runtime is expected to see, so rec_* values used downstream as record keys can collide. This is a recurring defect class — non-cryptographic PRNG + timestamp for what is functionally a key.

After:

const crypto = require('node:crypto');
function id() {
  return \`rec_\${crypto.randomUUID()}\`;
}

crypto.randomUUID() is a v4 UUID from a CSPRNG — 122 bits of entropy, collision probability negligible at any realistic ingest rate.

2. Empty-string vs absent

Before:

if (!doc.company_name && !doc.person_name) { /* 400 missing */ }
...
if (doc.company_name) envelopes.push(buildOrgEnvelope(...));
if (doc.person_name) envelopes.push(buildPersonEnvelope(...));

An empty string is falsy in JS, so a caller who intentionally sent {"company_name": ""} got back the 400 missing company_name and person_name — even though the field was provided (just empty). Envelope construction had the same defect: an empty-string company_name silently produced no organization envelope, so the response's envelope_count disagreed with what the caller thought they sent.

After: presence is checked with Object.prototype.hasOwnProperty.call(doc, 'company_name'), and a present-but-empty value gets its own distinct 400:

if (hasCompany && doc.company_name === '') {
  return sendJson(res, 400, { error: 'company_name provided but empty', request_id: requestId });
}

Envelopes now key off presence, not truthiness, matching the surface's contract that these fields are the shape identifiers for organization / person.

This is the same "empty-string treated as absent" pattern I've been finding in other services across the estate. Worth flagging at review time because it's a defect class rather than a one-off — anywhere a JS handler uses if (!x) on an optional user-supplied string, the same audit applies.

Test plan

  • node --check services/identity-firmographic-alpha-runtime/server.js passes.
  • Existing tests in services/identity-firmographic-alpha-runtime/ still pass; any tests that assert the exact 400 body for missing fields still match (unchanged for the truly-absent case).
  • POST /ingest/identity-firmographic with {"company_name": ""} returns 400 company_name provided but empty (was: 400 missing company_name and person_name).
  • POST /ingest/identity-firmographic with {} returns 400 missing company_name and person_name (unchanged).
  • POST /ingest/identity-firmographic with {"company_name": "Acme"} still produces a 202 with the organization envelope (unchanged behavior on the happy path).
  • Two ingest calls in the same millisecond produce distinct request_id values.

…h absent vs empty-string fields

Two defects in the /ingest/identity-firmographic runtime handler:

1. Id generation used Date.now() + Math.random().slice(2,10). Under
   concurrent ingest at the same millisecond the collision surface is
   the 8-hex-char random suffix — ~4.3B distinct values, i.e.
   birthday-collision territory well before the millions of requests
   this service is expected to see. Ids used as record keys must not
   collide; crypto.randomUUID() removes the failure mode entirely.

2. The validation on doc.company_name / doc.person_name used truthy
   checks. An empty string ('') is falsy in JavaScript, so a caller
   who intentionally sent {'company_name': ''} got the misleading
   'missing company_name and person_name' 400 back — the field WAS
   provided, just empty. Envelope construction had the same defect: an
   empty-string company_name silently produced no organization envelope.

   Fix: use Object.prototype.hasOwnProperty.call() to check presence,
   and emit distinct 400s ('company_name provided but empty' /
   'person_name provided but empty') for the present-but-empty case.
   Envelopes now key off presence, not truthiness, matching the
   surface's contract that these fields are the shape identifiers.

Same 'empty-string treated as absent' pattern I've been finding in
other services across the estate — worth flagging in review since it's
a defect class, not a one-off.
Copilot AI review requested due to automatic review settings July 30, 2026 05:59

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

This PR improves correctness in the identity-firmographic ingest runtime by strengthening request ID generation and by distinguishing between absent vs present-but-empty company_name / person_name fields during validation and envelope creation.

Changes:

  • Switches request/record ID generation to use crypto.randomUUID() instead of timestamp + Math.random().
  • Reworks validation/envelope routing to use field presence (hasOwnProperty) rather than truthiness.
  • Adds explicit 400 errors for present-but-empty string values for company_name / person_name.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


function id() {
return `rec_${Date.now()}_${Math.random().toString(36).slice(2, 10)}`;
return `rec_${crypto.randomUUID()}`;
Comment on lines +179 to +180
const hasCompany = Object.prototype.hasOwnProperty.call(doc, 'company_name');
const hasPerson = Object.prototype.hasOwnProperty.call(doc, 'person_name');
log('warn', 'validation_failed', { request_id: requestId, reason: 'missing company_name and person_name' });
return sendJson(res, 400, { error: 'missing company_name and person_name', request_id: requestId });
}
if (hasCompany && doc.company_name === '') {
log('warn', 'validation_failed', { request_id: requestId, reason: 'company_name provided but empty' });
return sendJson(res, 400, { error: 'company_name provided but empty', request_id: requestId });
}
if (hasPerson && doc.person_name === '') {
Comment on lines +195 to +196
if (hasCompany) envelopes.push(buildOrgEnvelope(doc, requestId));
if (hasPerson) envelopes.push(buildPersonEnvelope(doc, requestId));
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.

2 participants