Bug#1106428: Additional insights on the failing QUERY method test

[Jérémy Lal]

Le ven. 11 juil. 2025 à 23:45, Chris Hofstaedtler  a
écrit :

> Hi Jérémy,
>
> On Thu, Jun 05, 2025 at 12:17:13PM +0200, Jérémy Lal wrote:
> > Le jeu. 5 juin 2025 à 11:53, Santiago Vila  a écrit
> :
> >
> > > Good news from the github issue:
> > >
> > > "I'm gonna include 65c8380 in the v20 release"
> > >
> > > https://github.com/nodejs/node/issues/51562#issuecomment-2943090159
> >
> > I'm waiting a few hours to see if someone steps up, and will upload a
> > patched nodejs with that commit.
>
> I saw you uploaded a security release after this message, but the
> fix was not included if I read the changelog right.
>
> Could you take another look?
>

The 20.19.2 upload happened on may 18th, so not really "after this message".

I'm to upload a fixed nodejs with only selected patches, because upstream
keeps changing stuff
and 20.19.3 has not the level of quality that I expected.

Jérémy (from Brest !)

Bug#1106428: Additional insights on the failing QUERY method test

[Chris Hofstaedtler]

Hi Jérémy,

On Thu, Jun 05, 2025 at 12:17:13PM +0200, Jérémy Lal wrote:
> Le jeu. 5 juin 2025 à 11:53, Santiago Vila  a écrit :
> 
> > Good news from the github issue:
> >
> > "I'm gonna include 65c8380 in the v20 release"
> >
> > https://github.com/nodejs/node/issues/51562#issuecomment-2943090159
> 
> I'm waiting a few hours to see if someone steps up, and will upload a
> patched nodejs with that commit.

I saw you uploaded a security release after this message, but the 
fix was not included if I read the changelog right.

Could you take another look?

Thanks,
Chris

Bug#1106428: Additional insights on the failing QUERY method test

[Jérémy Lal]

Le jeu. 5 juin 2025 à 03:17, Santiago Vila  a écrit :

> Note: Beware: The package has autopkgtests, so maybe it's not as simple
> as disabling some tests. What can happen with the tests in testing
> if they run again now? Could they fail?
>

I believe this case is supported by the transition-to-testing logic.

Bug#1106428: Additional insights on the failing QUERY method test

[Jérémy Lal]

Le jeu. 5 juin 2025 à 11:53, Santiago Vila  a écrit :

> Good news from the github issue:
>
> "I'm gonna include 65c8380 in the v20 release"
>
> https://github.com/nodejs/node/issues/51562#issuecomment-2943090159


I'm waiting a few hours to see if someone steps up, and will upload a
patched nodejs with that commit.

Bug#1106428: Additional insights on the failing QUERY method test

[Santiago Vila]

Good news from the github issue:

"I'm gonna include 65c8380 in the v20 release"

https://github.com/nodejs/node/issues/51562#issuecomment-2943090159

Thanks.

Bug#1106428: Additional insights on the failing QUERY method test

[Santiago Vila]

El 5/6/25 a las 9:11, Jérémy Lal escribió:

The best fix would be in the embedded copy of llhttp in nodejs 20.19.2 - remove 
the HTTP QUERY support that was
added in 
https://github.com/nodejs/node/commit/eb25047b1b08180ed68dcefe2299682fbc7966ab 


This would avoid potentially hidden bugs in the nodejs-dependents packages.
Shall we reassign this bug to nodejs ?


Maybe, but in such case we might also forward this upstream. It would be 
troublesome
if Debian adds or removes something in our nodejs 20.19.2 and then they remove 
or add
it again in 20.19.3.

Thanks.

Bug#1106428: Additional insights on the failing QUERY method test

[Santiago Vila]

reassign 1106428 nodejs
found 1106428 20.19.2+dfsg-1
affects 1106428 src:node-body-parser
forwarded 1106428 
https://github.com/nodejs/node/issues/51562#issuecomment-2943008360
thanks

El 5/6/25 a las 11:21, Jérémy Lal escribió:

Le jeu. 5 juin 2025 à 11:20, Santiago Vila mailto:sanv...@debian.org>> a écrit :
Maybe, but in such case we might also forward this upstream. It would be 
troublesome
if Debian adds or removes something in our nodejs 20.19.2 and then they 
remove or add
it again in 20.19.3.

It already is ;)
https://github.com/nodejs/node/issues/51562#issuecomment-2943008360 



Ok. Let's see what they say.

Thanks.

Bug#1106428: Additional insights on the failing QUERY method test

[Jérémy Lal]

Le jeu. 5 juin 2025 à 11:20, Santiago Vila  a écrit :

> El 5/6/25 a las 9:11, Jérémy Lal escribió:
> > The best fix would be in the embedded copy of llhttp in nodejs 20.19.2 -
> remove the HTTP QUERY support that was
> > added in
> https://github.com/nodejs/node/commit/eb25047b1b08180ed68dcefe2299682fbc7966ab
> <
> https://github.com/nodejs/node/commit/eb25047b1b08180ed68dcefe2299682fbc7966ab
> >
> >
> > This would avoid potentially hidden bugs in the nodejs-dependents
> packages.
> > Shall we reassign this bug to nodejs ?
>
> Maybe, but in such case we might also forward this upstream. It would be
> troublesome
> if Debian adds or removes something in our nodejs 20.19.2 and then they
> remove or add
> it again in 20.19.3.
>

It already is ;)
https://github.com/nodejs/node/issues/51562#issuecomment-2943008360

Bug#1106428: Additional insights on the failing QUERY method test

[Jérémy Lal]

Le jeu. 5 juin 2025 à 02:28, Santiago Vila  a écrit :

> Thanks a lot for the investigation!
>
> So, it seems the logical thing to do here is to disable those failing
> tests,
> as they were not really intended to be run with nodejs 20.
>
> Is there any team preference about how to do that?
>
> I can think of at least three methods.
>
> Method 1: Skip also when running nodejs 20
>
> --- a/test/body-parser.js
> +++ b/test/body-parser.js
> @@ -82,7 +82,7 @@ describe('bodyParser()', function () {
> // update this implementation to run on supported versions of 21
> once they exist
> // upstream tracking https://github.com/nodejs/node/issues/51562
> // express tracking issue:
> https://github.com/expressjs/express/issues/5615
> -  return getMajorVersion(versionString) === '21'
> +  return getMajorVersion(versionString) === '20' ||
> getMajorVersion(versionString) === '21'
>   }
>
>   methods.slice().sort().forEach(function (method) {
>
> Method 2: Make the function to return true unconditionally, since this is
> targeted
> for trixie which will have nodejs 20.
>
> --- a/test/body-parser.js
> +++ b/test/body-parser.js
> @@ -82,7 +82,7 @@ describe('bodyParser()', function () {
> // update this implementation to run on supported versions of 21
> once they exist
> // upstream tracking https://github.com/nodejs/node/issues/51562
> // express tracking issue:
> https://github.com/expressjs/express/issues/5615
> -  return getMajorVersion(versionString) === '21'
> +  return true
>   }
>
>   methods.slice().sort().forEach(function (method) {
>
> Method 3: Assume that the function would return true in the place where
> the return value is used:
>
> --- a/test/body-parser.js
> +++ b/test/body-parser.js
> @@ -89,7 +89,7 @@ describe('bodyParser()', function () {
> if (method === 'connect') return
>
> it('should support ' + method.toUpperCase() + ' requests',
> function (done) {
> -if (method === 'query' && shouldSkipQuery(process.versions.node))
> {
> +if (method === 'query') {
> this.skip()
>   }
>   request(this.server)[method]('/')
>
>
> I could make a team upload if some authorized voice tells me which
> solution is best/preferred.
> (My personal preference would be method 2).
>
> Thanks.
>


The best fix would be in the embedded copy of llhttp in nodejs 20.19.2 -
remove the HTTP QUERY support that was
added in
https://github.com/nodejs/node/commit/eb25047b1b08180ed68dcefe2299682fbc7966ab

This would avoid potentially hidden bugs in the nodejs-dependents packages.
Shall we reassign this bug to nodejs ?

Bug#1106428: Additional insights on the failing QUERY method test

[Santiago Vila]

Note: Beware: The package has autopkgtests, so maybe it's not as simple
as disabling some tests. What can happen with the tests in testing
if they run again now? Could they fail?

Thanks.

Bug#1106428: Additional insights on the failing QUERY method test

[Santiago Vila]

Thanks a lot for the investigation!

So, it seems the logical thing to do here is to disable those failing tests,
as they were not really intended to be run with nodejs 20.

Is there any team preference about how to do that?

I can think of at least three methods.

Method 1: Skip also when running nodejs 20

--- a/test/body-parser.js
+++ b/test/body-parser.js
@@ -82,7 +82,7 @@ describe('bodyParser()', function () {
   // update this implementation to run on supported versions of 21 once 
they exist
   // upstream tracking https://github.com/nodejs/node/issues/51562
   // express tracking issue: 
https://github.com/expressjs/express/issues/5615
-  return getMajorVersion(versionString) === '21'
+  return getMajorVersion(versionString) === '20' || 
getMajorVersion(versionString) === '21'
 }
 
 methods.slice().sort().forEach(function (method) {


Method 2: Make the function to return true unconditionally, since this is 
targeted
for trixie which will have nodejs 20.

--- a/test/body-parser.js
+++ b/test/body-parser.js
@@ -82,7 +82,7 @@ describe('bodyParser()', function () {
   // update this implementation to run on supported versions of 21 once 
they exist
   // upstream tracking https://github.com/nodejs/node/issues/51562
   // express tracking issue: 
https://github.com/expressjs/express/issues/5615
-  return getMajorVersion(versionString) === '21'
+  return true
 }
 
 methods.slice().sort().forEach(function (method) {


Method 3: Assume that the function would return true in the place where the 
return value is used:

--- a/test/body-parser.js
+++ b/test/body-parser.js
@@ -89,7 +89,7 @@ describe('bodyParser()', function () {
   if (method === 'connect') return
 
   it('should support ' + method.toUpperCase() + ' requests', function (done) {

-if (method === 'query' && shouldSkipQuery(process.versions.node)) {
+if (method === 'query') {
   this.skip()
 }
 request(this.server)[method]('/')


I could make a team upload if some authorized voice tells me which solution is 
best/preferred.
(My personal preference would be method 2).

Thanks.

Bug#1106428: Additional insights on the failing QUERY method test

[Joachim Schwarm]

Hi,

after a little research, i found the following comment in the test case 
on node-body-parser:


> // Skipping HTTP QUERY tests on Node 21, it is reported in 
http.METHODS on 21.7.2 but not supported
> // update this implementation to run on supported versions of 21 once 
they exist

> // upstream tracking https://github.com/nodejs/node/issues/51562
> // express tracking issue: 
https://github.com/expressjs/express/issues/5615

> return getMajorVersion(versionString) === '21'

see: 
https://github.com/expressjs/body-parser/blob/1.20.3/test/body-parser.js#L80-L86


and despite being tagged with "dont-land-on-v20.x" in the upstream pr on 
nodejs (https://github.com/nodejs/node/pull/51719) it did in fact land 
on 20.19.2 (LTS) which is used by trixie (see 
https://github.com/nodejs/node/commit/eb25047b1b08180ed68dcefe2299682fbc7966ab 
and https://nodejs.org/en/blog/release/v20.19.2) on 2025-05-14.


Hope this helps

Best,
Joachim