fix: initialize strongest with null #145

Uzlopak · 2025-06-28T00:45:46Z

While implementing the latest draft to undici, nodejs/undici#4307, I found following issues:

Why is strongest initialized with an empty string, just to later contain an object? Why cant we just initialize it with null?

I think the word if was missing in Otherwise, |newAlgorithmIndex| and |currentAlgorithmIndex| are the

The point is, that we search for the strongest hash methods and the corresponding hashes. Without the if we get also based on the input we keep also weaker hashes, just based on the order of the input for the integrity string.

WIthout the if the second test would fail, just because the order is different.

  test('should return strongest sha512 /1', () => {
    const result = getStrongestMetadata([
      { alg: 'sha256', val: 'sha256-abc' },
      { alg: 'sha384', val: 'sha384-def' },
      { alg: 'sha512', val: 'sha512-ghi' }
    ])
    assert.deepEqual(result, [
      { alg: 'sha512', val: 'sha512-ghi' }
    ])
  })

  test('should return strongest sha512 /2', () => {
    const result = getStrongestMetadata([
      { alg: 'sha512', val: 'sha512-ghi' },
      { alg: 'sha256', val: 'sha256-abc' },
      { alg: 'sha384', val: 'sha384-def' }
    ])
    assert.deepEqual(result, [
      { alg: 'sha512', val: 'sha512-ghi' }
    ])
  })

Also

Preview | Diff

mozfreddyb · 2025-06-30T07:32:09Z

index.bs


-  1.  Let |result| be the empty set and |strongest| be the empty
-      string.
+  1.  Let |result| be the empty set and |strongest| be empty.


I have no strong feelings rather than doing what's common in other specs. My quick skim of of other specs makes me think that variables which are supposed to hold strings are initialized to "empty string" rather than "null" or "empty".

Given this is just a variable initialize that is immediately overwritten, I don't think this should make a difference, right?

Yeah, this should be reverted.

Well, the problem does not arise in programming languages where you dont have to initialize a variable with a datatype like php or javascript. But if you are e.g. programming c++ you declare strongest as string and set it as empty String and that you dont even need that string, suddenly you need to assign a struct, well how you gonna implement this?. I am not a c++ dev, but i think every c++ dev would just decide to deviate from the spec, and declare strongest to be struct and let it be empty.

In nodejs it can be a small performance issue. V8 engine could potentially not optimize the function due to the variable strongest not to be monomorphic, because the datatype changes. If we keep it to be undefined or null, which is in js our empty value, v8 has atleast a chance to optimize it.

Thanks. Let's keep the empty string then.
(Edit: I raced with your comment. Please ignore)

Then we should set it to null. Why shouldnt it be possible?

https://infra.spec.whatwg.org/#primitive-data-types

Yeah, that makes sense to me given what 2.2.2 does.

I misunderstood it to be a string in the end. You're right, it should be null initially and then eventually assigned a struct. Can you rewrite your patch to say null rather than empty?

mozfreddyb · 2025-06-30T07:45:11Z

index.bs

-      7.  Otherwise, |newAlgorithmIndex| and |currentAlgorithmIndex| are the
+      7.  Otherwise, if |newAlgorithmIndex| and |currentAlgorithmIndex| are the


Thanks, looks like this was a missing word indeed.

Are you sure? How else can they compare besides greater than, less than, and equal to?

Sure you could also read that as a statement "Otherwise they are the same, so append". Either way, this shouldn't change the logic.

Though, I am not sure what the test case difference @Uzlopak meant. Maybe he can help me understand.

In the wording of the spec it is not clear if you want that i implement an if condition to check that both variables are equal or if you just state in step 7 that we are left with the case that both variables are equal and we have to process as defined in 7.

When i implemented it on the weekend i assessed that the "if" is missing.

Imho: In a Security spec it should be not so much wiggle room in interpreting an Instruction.
Especially as there can arise a false implementation, which could be undetected even if you write a test, see initial post.

I think adding an "if" is a big change as it implies it was not a statement of fact.

I don't think there's any wiggle room here, though it could be modernized to use Assert instead. There might be a bug, but that's still unclear.

So if it is a statement of fact that at step 7. |newAlgorithmIndex| and |currentAlgorithmIndex| are the same, then the implementation is as follows:

/** * @param {MetadataList} metadataList * @returns {MetadataList} The strongest hash algorithm from the metadata list. */ function getStrongestMetadata (metadataList) { // 1. Let result be the empty set and strongest be the empty string. const result = [] /** @type {Metadata|string} */ let strongest = '' // 2. For each item in set: for (const item of metadataList) { // 1. Assert: item["alg"] is a valid SRI hash algorithm token. assert(isValidSRIHashAlgorithm(item.alg), `Invalid SRI hash algorithm: ${item.alg}`) // 2. If result is the empty set, then: if (result.length === 0) { // 1. Append item to result. result.push(item) // 2. Set strongest to item. strongest = item // 3. Continue. continue } // 3. Let currentAlgorithm be strongest["alg"], and currentAlgorithmIndex be // the index of currentAlgorithm in the valid SRI hash algorithm token set. const currentAlgorithm = /** @type {Metadata} */ (strongest).alg const currentAlgorithmIndex = validSRIHashAlgorithmTokenSet.indexOf(currentAlgorithm) // 4. Let newAlgorithm be the item["alg"], and newAlgorithmIndex be the // index of newAlgorithm in the valid SRI hash algorithm token set. const newAlgorithm = item.alg const newAlgorithmIndex = validSRIHashAlgorithmTokenSet.indexOf(newAlgorithm) // 5. If newAlgorithmIndex is less than 0, then continue. if (newAlgorithmIndex < 0) { continue // 6. Otherwise, if newAlgorithmIndex is greater than // currentAlgorithmIndex: } else if (newAlgorithmIndex > currentAlgorithmIndex) { // 1. Set strongest to item. strongest = item // 2. Set result to « item ». result[0] = item result.length = 1 // 7. Otherwise, newAlgorithmIndex and currentAlgorithmIndex are the same // value. Append item to result. } else { result.push(item) } } // 3. Return result. return result }

If we say, that we are NOT sure, that `|newAlgorithmIndex| and |currentAlgorithmIndex| are the same and we have to check with an if-condition we get following implementation.

/** * @param {MetadataList} metadataList * @returns {MetadataList} The strongest hash algorithm from the metadata list. */ function getStrongestMetadata (metadataList) { // 1. Let result be the empty set and strongest be the empty string. const result = [] /** @type {Metadata|string} */ let strongest = '' // 2. For each item in set: for (const item of metadataList) { // 1. Assert: item["alg"] is a valid SRI hash algorithm token. assert(isValidSRIHashAlgorithm(item.alg), `Invalid SRI hash algorithm: ${item.alg}`) // 2. If result is the empty set, then: if (result.length === 0) { // 1. Append item to result. result.push(item) // 2. Set strongest to item. strongest = item // 3. Continue. continue } // 3. Let currentAlgorithm be strongest["alg"], and currentAlgorithmIndex be // the index of currentAlgorithm in the valid SRI hash algorithm token set. const currentAlgorithm = /** @type {Metadata} */ (strongest).alg const currentAlgorithmIndex = validSRIHashAlgorithmTokenSet.indexOf(currentAlgorithm) // 4. Let newAlgorithm be the item["alg"], and newAlgorithmIndex be the // index of newAlgorithm in the valid SRI hash algorithm token set. const newAlgorithm = item.alg const newAlgorithmIndex = validSRIHashAlgorithmTokenSet.indexOf(newAlgorithm) // 5. If newAlgorithmIndex is less than 0, then continue. if (newAlgorithmIndex < 0) { continue // 6. Otherwise, if newAlgorithmIndex is greater than // currentAlgorithmIndex: } else if (newAlgorithmIndex > currentAlgorithmIndex) { // 1. Set strongest to item. strongest = item // 2. Set result to « item ». result[0] = item result.length = 1 // 7. Otherwise, newAlgorithmIndex and currentAlgorithmIndex are the same // value. Append item to result. } else if (newAlgorithmIndex === currentAlgorithmIndex) { result.push(item) } } // 3. Return result. return result }

And then we get back to the original post of mine.

If we pass

[ { alg: 'sha256', val: 'sha256-abc' }, { alg: 'sha384', val: 'sha384-def' }, { alg: 'sha512', val: 'sha512-ghi' } ]

as payload for getStrongestMetaaata, then both implementation return [{ alg: 'sha512', val: 'sha512-ghi' }] as result.

But if we pass

[ { alg: 'sha512', val: 'sha512-ghi' }, { alg: 'sha256', val: 'sha256-abc' }, { alg: 'sha384', val: 'sha384-def' } ]

as payload for getStrongMetadata then the original spec will return

[ { alg: 'sha512', val: 'sha512-ghi' }, { alg: 'sha256', val: 'sha256-abc' }, { alg: 'sha384', val: 'sha384-def' } ]

In step 5 we check if the newIndex is lower than 0, which means, that the algorithm isnt supported
In step 6 we check if the newIndex is greater than currentIndex, OK, makes sense
and in step 7 we expect it to be equal, but we never checked if newIndex is lower than currentIndex.

In step 5 you do if (newAlgorithmIndex < 0) { continue, I think you'd need to do if (newAlgorithmIndex < currentAlgorithmIndex) { continue.

If you check for newAlgorithmIndex < currentAlgorithmIndex, then newAlgorithmIndex > currentAlgorithmIndex, the only remaining obvious case is equality.

@mozfreddyb
yeah, also in 2.1. you assert, that the algorithm is valid. So currentAlgorithmIndex can never be < 0, so 2.5 is never happening.

OK! I guess I have here a discrepancy between the draft from april and from may, as it is clearly fixed already in .2.5.

I didnt see that.

I revert that if.

mozfreddyb · 2025-06-30T07:57:00Z

@annevk Can you briefly chime in on the empty string / null string question above? I don't have a strong opinion but couldn't find an authoritative answer

Uzlopak · 2025-06-30T10:56:35Z

I connected my account to w3c.

mozfreddyb · 2025-06-30T13:37:37Z

Uzlopak did not make IPR commitments for this group. To make the IPR commitments, Uzlopak should join the Web Application Security Working Group .

I think I can waive the commit as non-substantive, as this clarifies the algorithm but does not change the logical behavior

w3cbot · 2025-06-30T13:37:40Z

mozfreddyb marked as non substantive for IPR from ash-nazg.

mozfreddyb · 2025-06-30T13:38:31Z

Thank you @Uzlopak.

SHA: 679d685 Reason: push, by mozfreddyb Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

fix: get strongest metadata add missing if

72ab03a

mozfreddyb reviewed Jun 30, 2025

View reviewed changes

mozfreddyb requested a review from annevk June 30, 2025 07:56

Update index.bs

faa726d

Uzlopak changed the title ~~fix: get strongest metadata add missing if, initialize strongest with empty~~ fix: initialize strongest with null Jun 30, 2025

mozfreddyb approved these changes Jun 30, 2025

View reviewed changes

mozfreddyb merged commit 679d685 into w3c:main Jun 30, 2025
2 checks passed

github-actions bot added a commit that referenced this pull request Jun 30, 2025

fix: initialize strongest with null (#145)

5fc38b8

SHA: 679d685 Reason: push, by mozfreddyb Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

		7. Otherwise, \|newAlgorithmIndex\| and \|currentAlgorithmIndex\| are the
		7. Otherwise, if \|newAlgorithmIndex\| and \|currentAlgorithmIndex\| are the

fix: initialize strongest with null #145

fix: initialize strongest with null #145

Uh oh!

Conversation

Uzlopak commented Jun 28, 2025 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mozfreddyb Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uzlopak Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mozfreddyb commented Jun 30, 2025

Uh oh!

Uzlopak commented Jun 30, 2025

Uh oh!

mozfreddyb commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

w3cbot commented Jun 30, 2025

Uh oh!

mozfreddyb commented Jun 30, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uzlopak commented Jun 28, 2025 •

edited by pr-preview bot

Loading

mozfreddyb Jun 30, 2025 •

edited

Loading

Uzlopak Jun 30, 2025 •

edited

Loading

mozfreddyb commented Jun 30, 2025 •

edited

Loading