New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update SPDX Expression Parsing #719
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☝️ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 do these paths require the ../__tests__/
prefix or would a fixtures/
prefix work too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
expect(spdx.satisfies(license, expr)).toBe(false) | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☝️ 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-satisfies
is 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.js
and uses@onebeyond/spdx-license-satisfies
instead to check whether an SPDX license satisfies an expression or not: TheMIT
license 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.ts
noting the things we still need to support.tsconfig.json
to fix a duplicate entry in the compiler options.