k6 icon indicating copy to clipboard operation
k6 copied to clipboard

Published 20 hours ago • verified publisher icon grafana

Fix : add warning in RSA PSS webcrypto

Open vincentandreas opened this issue 7 months ago • 5 comments
trafficstars

What?

Implement warning in RSA PSS, when given salt length = 0, in Sign / Verify function

Why?

It improves UX and makes implementation predictable.

Checklist

  • [x] I have performed a self-review of my code.
  • [ ] I have commented on my code, particularly in hard-to-understand areas.
  • [ ] I have added tests for my changes.
  • [x] I have run linter and tests locally (make check) and all pass.

Checklist: Documentation (only for k6 maintainers and if relevant)

Please do not merge this PR until the following items are filled out.

  • [ ] I have added the correct milestone and labels to the PR.
  • [ ] I have updated the release notes: link
  • [ ] I have updated or added an issue to the k6-documentation: grafana/k6-docs#NUMBER if applicable
  • [ ] I have updated or added an issue to the TypeScript definitions: grafana/k6-DefinitelyTyped#NUMBER if applicable

Related PR(s)/Issue(s)

https://github.com/grafana/k6/issues/4265

Yes

vincentandreas avatar Mar 26 '25 17:03 vincentandreas

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Mar 26 '25 17:03 CLAassistant

Sure @mstoykov I'll wait other discussion related to this, thx for your suggestion

vincentandreas avatar Mar 27 '25 10:03 vincentandreas

After over a week it seems like the agreement is to have this as an exception instead of warning.

You can do that by returning an error - Sobek (the JS VM) will take this as an exception. I would also prefer if there are some tests added to check this.

mstoykov avatar Apr 02 '25 15:04 mstoykov

:wave: @vincentandreas

Please rebase on latest master as https://github.com/grafana/k6/pull/4701 is needed to see if the test are working.

This was the problem we were having but now it should fail on test failure.

Sorry for the inconvience and the time it took, but it was kind of hard to figure out why was thsi working before :(

mstoykov avatar Apr 17 '25 09:04 mstoykov

Hi @vincentandreas, will you be able to work on the changes? Thanks!

~PS: test-current-cov fails, and I'm working on a fix: #4837~ Fixed.

inancgumus avatar Jun 06 '25 16:06 inancgumus

Hi @vincentandreas , first - thank you for the work you have done :bow:

This PR has been open for a while now. We would like to get a fix for the issue, but as discussed before, instead of logging we would like to throw an exception. And we would also would like a test about it.

If you are not able to continue, that is fine. We can take over, but prefer to let you finish the work. There is no rush on getting this done instantly, we just want to make certain that we do not take over while you are working it or expected some feedback from us.

mstoykov avatar Jun 26 '25 12:06 mstoykov

I will close this PR, due to inactivity :(

Thank you for bringing this up and helping figureing out the correct approach.

Given the low priority of the issue and that it seems like a good first issue we will be leaving it open instead of taking over. At least for now.

If you @vincentandreas or somebody else wants to work on, you are welcome to open a PR.

mstoykov avatar Jul 10 '25 12:07 mstoykov