dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.37k stars 9.99k forks source link

Fix warning "Could not parse Jtokens from ...package-lock.json file." #22243

Closed wtgodbe closed 4 years ago

wtgodbe commented 4 years ago

Part of https://github.com/dotnet/aspnetcore/issues/22240

Happens during component detection. Whole warning text:

[warning]Could not parse Jtokens from F:\workspace_work\1\s\artifacts\tmp\Release\ContainerSigning\2455\content\ReactRedux-CSharp\ClientApp\package-lock.json file.

[warning]Could not parse Jtokens from F:\workspace_work\1\s\artifacts\tmp\Release\ContainerSigning\2407\content\React-CSharp\ClientApp\package-lock.json file.

[warning]Could not parse Jtokens from F:\workspace_work\1\s\artifacts\tmp\Release\ContainerSigning\2315\content\Angular-CSharp\ClientApp\package-lock.json file.

dougbu commented 4 years ago

:eyes:

dougbu commented 4 years ago

Are you sure the vulnerability warnings are in any way related to the inability to parse the lock file?

wtgodbe commented 4 years ago

Hmm, actually maybe not - I thought the number of 'lock.json' errors corresponded to the number of Component Governance errors, but looks like that's not the case. I'll separate the 2 out.

dougbu commented 4 years ago

@mkArtakMSFT one complete warning text is

##[warning]Could not parse Jtokens from F:\workspace\_work\1\s\artifacts\tmp\Release\ContainerSigning\2369\content\Angular-CSharp\ClientApp\package-lock.json file.

[VERBOSE] Could not read component details from file F:\workspace\_work\1\s\artifacts\tmp\Release\ContainerSigning\2369\content\Angular-CSharp\ClientApp\package-lock.json 

[INFO] LogFailedReadingFile logged InvalidPackageJsonException: There must be a package.json file at 'F:\workspace\_work\1\s\artifacts\tmp\Release\ContainerSigning\2369\content\Angular-CSharp\ClientApp\package-lock.json' for components to be registered

Questions:

  1. Why do we ship a package-lock.json file and not a package.json file?
  2. Is anything other than the "missing" package.json file an issue for the component governance system?
    • E.g. does the package-lock.json file contain comments the system can't parse?
  3. Should we exclude artficacts/tmp/ from component governance checks?
mkArtakMSFT commented 4 years ago

@dougbu ,answering your questions here:

  1. @javiercn do you know why we ship package-lock.json? If we ship only package.json, wouldn't the build automatically generate the lock file, correct?
  2. According to that error, it seems that we have to have package.json as that's what the CG is looking for. So the above answer should resolve this too, I hope.
  3. What is in the artifacts/tmp/ ? We should just chat about this instead. Please put something on my calendar to go over this together.
javiercn commented 4 years ago

@mkArtakMSFT we want to keep the set of bits we ship stable and deterministic over time with what we've verified. Otherwise we run the risk of an updated version silently installing after we've shipped and that causing issues.

mkArtakMSFT commented 4 years ago

Thanks, yes, I remember now! Exactly this! @dougbu, because of this, if ignoring the artifacts/tmp is the way to avoid this error, given that we won't add package.json instead of the package-lock.json, we should do it.

dougbu commented 4 years ago

artifacts/tmp is the folder on the CI where $env:Temp points. Removing it from the governance check shouldn't break anything because the package-lock.json file in the src/ directory should already have been checked.

@wtgodbe where's the document describing how to add exclusions to the component governance checks?

wtgodbe commented 4 years ago

@dougbu it's an internal doc, I just sent you the .pdf on teams

dougbu commented 4 years ago

FYI the component governance task was auto-injected into all of our build jobs, including Code Check and the Test jobs. I think I changed the policy appropriately but haven't noticed a change yet. 🤞

dougbu commented 4 years ago

This is going to take more time because the component detection build task's support for excluding folders from the search seems to be broken. I'm going to test scanning our src/, artifacts/bin, artifacts/installers, artifacts/obj, and artifacts/packages folders. Will leave out bin/ and obj/ if they cause any noise.

dougbu commented 4 years ago

eb33b9657bac