Skip to content

fix(generateUuid): never mint an all-digit id - #196

Open
cbrauchli wants to merge 2 commits into
apache:masterfrom
cbrauchli:fix/generate-uuid-all-digits
Open

cbrauchli wants to merge 2 commits into
apache:masterfrom
cbrauchli:fix/generate-uuid-all-digits

Conversation

@cbrauchli

Copy link
Copy Markdown

Motivation and Context

generateUuid() builds an id from the first 24 hex characters of a random UUID, so sometimes it generates only decimal digits.

The pbxproj grammar reads such an id as a number whenever no comment follows it:

IntegerValue
  = !Alpha number:Digit+ !NonTerminator
    { return parseInt(number.join(''), 10) }

NonTerminator
  = [^;,\n]

Several references this library writes have no trailing comment:

  • the buildConfigurationList and productReference of a target from addTarget();
  • the remoteGlobalIDString of the PBXContainerItemProxy from addTargetDependency().

When one of those ids is all digits, the first write is still correct. Parsing that file returns the reference as a number, though, and the next write prints it in scientific notation:

buildConfigurationList = 1.2345678901243459e+23;

The reference then names no object. Any tool that writes a project and later parses and writes it again can hit this. For example, Expo's prebuild re-reads the project it has just written during its entitlements step.

Description

generateUuid() now mints again when the id is all digits, the same way it already does on a collision. Other ids are unaffected.

The underlying problem is in the parser and the writer. A project produced by another tool can contain an all-digit id too, and a round trip through this library corrupts that one the same way. A complete fix would be one of these:

  • have the parser keep an unquoted run of digits as a string when it can't round-trip as a number;
  • have the writer quote string values that would read back as numbers.

Both change how existing projects parse or print. This PR keeps to the minimal change that stops the library from creating such ids. I'm happy to follow up with a parser or writer change if you prefer that approach.

Reproduction

Run from a checkout of this repository:

const crypto = require('crypto');
const fs = require('fs');
const os = require('os');
const path = require('path');
const xcode = require(process.cwd());

const file = path.join(fs.mkdtempSync(path.join(os.tmpdir(), 'xcode-')), 'project.pbxproj');
fs.copyFileSync('test/parser/projects/full.pbxproj', file);

const project = xcode.project(file);
project.parseSync();

// About 1 random id in 60,000 is all digits. addTarget() mints the target's id
// first and its configuration list's id second; make the second one all digits.
const randomUUID = crypto.randomUUID;
let calls = 0;
crypto.randomUUID = () => (++calls === 2 ? '12345678-9012-4345-8678-901234567890' : randomUUID());
const target = project.addTarget('Repro', 'app_extension');
crypto.randomUUID = randomUUID;

const listId = target.pbxNativeTarget.buildConfigurationList;
console.log('buildConfigurationList minted:', JSON.stringify(listId));
fs.writeFileSync(file, project.writeSync());

const reread = xcode.project(file);
reread.parseSync();
const value = reread.pbxNativeTargetSection()[target.uuid].buildConfigurationList;
console.log('after write + parse:          ', JSON.stringify(value), `(${typeof value})`);
console.log('configuration list found:     ', Boolean(reread.pbxXCConfigurationList()[value]));
fs.writeFileSync(file, reread.writeSync());
const line = fs
    .readFileSync(file, 'utf8')
    .split('\n')
    .find((l) => /buildConfigurationList = [^ ]+;$/.test(l));
console.log('written again:                ', line.trim());

On master (940d6db):

buildConfigurationList minted: "123456789012434586789012"
after write + parse:           1.2345678901243459e+23 (number)
configuration list found:      false
written again:                 buildConfigurationList = 1.2345678901243459e+23;

With this change:

buildConfigurationList minted: "93C3B773B2574223BA4BB59E"
after write + parse:           "93C3B773B2574223BA4BB59E" (string)
configuration list found:      true
written again:                 buildConfigurationList = 93C3B773B2574223BA4BB59E;

Testing

  • test/pbxProject.js gets a new test, should not return an id made only of digits. It mocks crypto.randomUUID() to return an all-digit UUID and then a normal one, and checks that generateUuid() returns the second. Without the fix, generateUuid() returns 123456789012434586789012 and the test fails.
  • npm test (lint and all 436 tests) passes on Node 22.

The parser's IntegerValue rule reads an unquoted run of digits that is
followed directly by ";" or "," as a number. A value written without a
trailing comment, such as the buildConfigurationList or productReference
of a target from addTarget(), or the remoteGlobalIDString of a
PBXContainerItemProxy from addTargetDependency(), therefore comes back
from a write and a parse as a number when its id is all digits. The
next write prints it in scientific notation (1.2345678901243459e+23),
and the reference names no object.

About 1 random id in 60,000 is all digits, so generateUuid() now mints
again when it gets one, as it already does on a collision.
Mocks crypto.randomUUID() to return an all-digit UUID and then a normal
one, and checks that generateUuid() returns the second.
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.42%. Comparing base (940d6db) to head (5faa9a1).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #196      +/-   ##
==========================================
+ Coverage   97.41%   97.42%   +0.01%     
==========================================
  Files          52       52              
  Lines       11164    11178      +14     
==========================================
+ Hits        10875    10890      +15     
+ Misses        289      288       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@erisu erisu left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I tried digging into this issue a little and can confirm seeing the scientific notation in certain cases.

From what I can see, the issue comes from the parser.

For example, if an all-numeric UUID is followed by a comment, it is parsed as a string, but if it ends with a semicolon (;), its would be parsed as a number. This is because of the combination of the IntegerValue and NonTerminator, which you pointed out in the PR description.

I also agree that the underlying issue is the parser itself. But I think the writer, on the other hand, isnt the apart of the root problem. When I reviewed the outputted project.pbxproj file, it appeared correct.

All in all, I believe generating an all-numeric UUID is relatively low. But, I think this PR would mitigate the issue by avoiding all-numeric UUIDs.

There might be a performance hit if regeneration is triggered frequently, but it might be a rare case.

I also want to mention that if a project already contains an all-numeric UUID in the pbxproj file, the parsing issue will still occur. This PR does not resolve the parser. I think a long term goal would be to fix the parser, as you mentioned as one of the options, but would require heavy testing to ensure that the changes doesn't break other areas.

As for this PR, I think its OK

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.

3 participants