Freecodecamp: Broken tests on multiple challenges in the Information Security with HelmetJS

[obsessedyouth]

Describe your problem and how to reproduce it:
Multiple challenge tests fail for no obvious reasons related to the challenge -

1. Mitigate the Risk of Clickjacking with helmet.frameguard()
2. Prevent IE from Opening Untrusted HTML with helmet.ieNoOpen()
3. Disable DNS Prefetching with helmet.dnsPrefetchControl()

Add a Link to the page with the problem:
https://glitch.com/edit/#!/busy-rambunctious-tern
this server fails the tests for reasons beyond my understanding.
I tried to replicate the server on a different service and tweak the tests and I still don't understand the cause.
https://repl.it/@obsessedyouth/DiligentAnchoredObservation#package.json

Tell us about your browser and operating system:

• Browser Name:
  Google Chrome
• Browser Version:
  Version 83.0.4103.116 (Official Build) (64-bit)
• Operating System:
  Windows 10 Enterprise

If possible, add a screenshot here (you can drag and drop, png, jpg, gif, etc. in this box):

[Sky020]

I can confirm this is an issue. It appears the likely cause is helmetjs renaming their middleware.

These are the new name:
frameguard -> xFrameOptionsMiddleware
ienoopen -> xDownloadOptionsMiddleware
dnsPrefetchControl -> xDnsPrefetchControlMiddleware

I cannot confirm anything wrong with the CSP tests.

[lasjorg]

Some of the function names changed
https://github.com/helmetjs/helmet/blob/master/index.ts#L136

module aliases tests
https://github.com/helmetjs/helmet/blob/master/test/index.test.ts#L16

Shouldn't we get this fixed sooner rather than later? Unless someone jumps on this fairly quickly I'd suggest removing the first timers only label.

Side note: The way the tests are done (_router.stack) seems like it would be pretty error/breakage-prone.

[Sky020]

@lasjorg You are right, it would be best to get it fixed sooner. I just expected someone to jump on it, as it is quick and easy.

The tests are done in a way that keeps us on our toes, regarding changes.

[obsessedyouth]

Is it okay if I raised a PR to fix this? or is someone else on it? I haven't really dug through the fcc code base so I'm not sure where the tests live but now that I know the root cause it seems simple enough.

[Sky020]

@obsessedyouth , No one has mentioned to be working on it. So, go ahead. Here is where the tests live: https://github.com/freeCodeCamp/freeCodeCamp/tree/master/curriculum/challenges/english/09-information-security/information-security-with-helmetjs

[RandellDawson]

Just to confirm, an old version of HelmetJS we specified in the project's package.json was failing these tests? If the old version was not failing, then why are we having to rewrite the tests? I have not dug to deep, but thought I would get some clarification before starting to change things that would break projects that already passed the challenges with an older version.

[lasjorg]

@RandellDawson The starter boilerplate does not have helmetjs in the package.json, the first challenge is to add it.

Not so sure locking the boilerplate to a specific version is the right choice, although it is certainly an option.

[lasjorg]

Also just as an aside. If we want to keep backward compatibility with projects made before this change we have to check for both the old and new function names. Not sure what the consensus on this is?

[Sky020]

If we must, I would suggest checking for both old and new, as _lasjorg_ mentioned. Originally, I assumed backwards compatibility was not a problem, because this is related to the lesson content, and not projects. So, all a user currently on them has to do, is update the version of Helmetjs in their package.json.

[lasjorg]

I'm just calling the finished version of the code a camper would end up with from this section of challenges a "project". I know it isn't really a project.

Sure it is easy to fix, but you have to know what the problem is and to update the dependencies.

I guess there isn't any expectation that the code used to pass the challenges will be accessible or remain valid. Once you have passed all the challenges you can delete the code and still have a valid passing section of challenges. So it doesn't matter much (from a validation point of view) if the code doesn't pass at a later date. But from a camper's point of view, it may be confusing why code that was passing now doesn't.

[Al3busse]

In order to pass the test Mitigate the Risk of Clickjacking with helmet.frameguard() i had to go back to version: 2.3.0
Then at the test Ask Browsers to Access Your Site via HTTPS Only with helmet.hsts() i had to use the lastest version atm 3.23.3
Then at the test Disable DNS Prefetching with helmet.dnsPrefetchControl() i had to go back to version 2.3.0

[obsessedyouth]

Hi @lasjorg @Sky020 @RandellDawson can you guys please reach a consensus about what will be done regarding this issue? It seems it is starting to affect more campers. I think my PR addresses this and backward compatibility is extremely unlikely since I doubt anyone would try to attempt a lesson with an older version of a project using helmet.js. However if you don't find it a satisfactory solution could you please suggest a better fix?

[lasjorg]

I think my PR addresses this and backward compatibility is extremely unlikely since I doubt anyone would try to attempt a lesson with an older version of a project using helmet.js.

@obsessedyouth Fixing the current issue is definitely a lot more important than any backward compatibility considerations. I think it might be fine to break old "projects".

I doubt that there are many people that will go back and re-run the tests on their old code. However, it is possible that some campers may have started doing the challenges and then moved on to something else, or took a break, and when they return back their starter code will be broken.

I just wanted to point out that updating the tests would break any older starter code. But like I said backward compatibility issues are not as important as fixing the current one.

[RandellDawson]

@moT01 @ojeytonwilliams Any thoughts on this issue?

[moT01]

How about rewriting the lesson that installs the package to make users install the specific version we were using when the lessons were written? This will keep everything compatible and save us from running into this issue again if something else in the package changes. I don't like teaching older versions of things, but it may be a good way to make sure things keep working - I think that would work anyway.

[RandellDawson]

@moT01 That is a good point. Many companies purposely do not upgrade a package, to avoid breaking existing code.

[obsessedyouth]

Well, this was an unexpected outcome but I'm happy this will get addressed either way.

[ojeytonwilliams]

Yep, I agree with Tom.

The packages should definitely get pinned to specific versions and get updated when a contributor intends to move to new versions. Ideally they should be updated to the latest stable builds, but it's important that the process is controlled.

[lasjorg]

I honestly think the real problem here is how we are testing for the middleware.

1. Depending on the internals of Express, especially internals declared using underscore (_router.stack) is surely bound to cause issues.

2. Looking at internal function names of middleware and not the publicly exposed middleware methods. The HelmetJS middleware method names have not changed (that would be a major breaking change). Only the internal function name assigned to the middleware properties has changed (i.e. aliasing).

Ideally, we should able to check for the middleware by looking at publicly exposed APIs and not depend on the internal function names. I'm just not sure how you would normally test/check for properly mounted middleware. I'm guessing most just check the functionality and look to see if the middleware is performing as expected and not check for the mounting itself.

I'd still expect there to be some way to perform testing for properly mounted middleware without having to do what we are doing.

[lasjorg]

@RandellDawson @ojeytonwilliams @moT01

So what is the plan here? Should we update the challenge description to ask the camper to install a specific version or do we just abandon backward compatibility and go with the PR from obsessedyouth?

BTW, I believe I tested all the challenges at one point with version 3.21.3 and it worked. Just an FYI

[moT01]

I think there was pretty good support for rewriting the lesson to use a specific version of helmet - If the current tests all pass with 3.21.3, like @lasjorg mentioned - we could probably go with that one. It has the dual benefit of backwards compatibility and stopping this issue from coming up again if/when helmet changes something else. So I would say to pin helmet to that version if it works. I believe this would only required a small change to the instructions of the first lesson in that section, and probably add a test for it as well - and probably add the version to the boilerplate as well.