Update SPDX Expression Parsing #719
Conversation
I don't think we were exercising this before, but the type won't match.
| @@ -197,7 +209,7 @@ function parseConfigFile(configData: string): ConfigurationOptionsPartial { | |||
| } | |||
| return data | |||
| } catch (error) { | |||
| throw error | |||
| throw new Error(`error validating config: ${error}`) | |||
There was a problem hiding this comment.
| throw new Error(`error validating config: ${error}`) | |
| throw new Error(`error validating config: ${(error as Error).message}`) |
Worth casting to get the message here? Not sure what the output would be if this is an object.
There was a problem hiding this comment.
The reason to cast is to please the TS type checker, because error is only defined inside the catch we need to give it some hints so it knows it can call message to get the error body.
There was a problem hiding this comment.
☝️ I think what Mitchell is pointing out is that we're missing the message call after the cast there?
| const config = await readConfig() | ||
| expect(config.fail_on_severity).toEqual('critical') | ||
| }) | ||
|
|
||
| test('it raises an error when given an unknown severity in an external config file', async () => { | ||
| setInput('config-file', './__tests__/fixtures/invalid-severity-config.yml') | ||
| setInput('config-file', '../__tests__/fixtures/invalid-severity-config.yml') | ||
| await expect(readConfig()).rejects.toThrow() |
There was a problem hiding this comment.
🤔 do these paths require the ../__tests__/ prefix or would a fixtures/ prefix work too?
There was a problem hiding this comment.
I'm not entirely sure why (maybe jest?), but the path needs to be relative to src/, hence the change from ./ to ../:
[FAIL] it accepts an external configuration filename
Unable to fetch or parse config file: ENOENT: no such file or directory, open '/Users/febuiles/w/dr-action/src/fixtures/no-licenses-config.yml'
|
|
||
| expect(spdx.satisfies(license, expr)).toBe(false) | ||
| }) | ||
| }) |
| // usually means a discrepancy in ESM/CJS. | ||
| if (e instanceof TypeError) { | ||
| throw new Error('spdx: invalid expression parser') | ||
| } |
There was a problem hiding this comment.
🤔 should the error message instead be spdx: invalid expression or something? the parser isn't the problem here, it's the inputs right?
Also under which error conditions is it safe to swallow the error and message here and just return false as in line 22?
There was a problem hiding this comment.
should the error message instead be spdx: invalid expression or something? the parser isn't the problem here, it's the inputs right?
It's the parser in the sense that TypeError is what's raised when there's an error loading the parser. I have not seen these errors outside of local development, where they're common if you forget to npm i after bootstrapping the project.
Also under which error conditions is it safe to swallow the error
None, if this fails we should abort the run.
There was a problem hiding this comment.
☝️ same here does that sync with your understanding @hmaurer ? we just bail if we can't parse a license, even if there may be vuln pkgs etc. found in this run?
again, I have little context/depth on DR Action at present, so if this is a non-concern, feel free to ignore 👍
|
Couple questions, but this is looking good so far, thanks! 🍻 |
Closes #263
Closes #670
Closes #575
Closes #635
Context
Since we introduced SPDX licenses around 2022, we've had issues dealing with SPDX expression validations due to the library we use for checking whether one expression satisfies another one.
Folks reached out to the maintainer in 2022 to fix some of these changes, but set a clear direction that does not fit our purposes anymore. The
@onebeyond/spdx-license-satisfiesis a fork of the original project, created by people who encountered the same issues as us.Changes
This PR moves the Action away from
spdx-satisfies.jsand uses@onebeyond/spdx-license-satisfiesinstead to check whether an SPDX license satisfies an expression or not: TheMITlicense satisfies the expressionMIT OR GPL-2.0, but it does not satisfyMIT AND GPL-2.0.In the process of making these changes I:
spdx.ts.spdx.tsnoting the things we still need to support.tsconfig.jsonto fix a duplicate entry in the compiler options.