veraison / services

Attestation verification services based on Veraison components
Apache License 2.0
26 stars 14 forks source link

fix!(scheme, api): correctly handle media type parameters with absolut… #260

Closed thomas-fossati closed 3 months ago

thomas-fossati commented 3 months ago

When the "profile" parameter value is an absolute URI, it must be quoted.

The definition of parameter-value in Appendix A of RFC9110 allows both token and quoted-string, but only the latter is compatible with absolute URI syntax, which includes at least the (reserved) ':' following the scheme name.

(Note that the OID form for the profile does not need to be quoted.)

This change also makes sure that both the media types we read and those we write are validated and normalised.

Fix #257

yogeshbdeshpande commented 3 months ago

@thomas-fossati is this a breaking change and does it need any change we refer profile in cocli corim submit1 andevcli`

setrofim commented 3 months ago

Note that end-to-end scripts and the walkthrough (as well as ny other documentation that has examaples of specifying a media type needs to updated as well.

thomas-fossati commented 3 months ago

@thomas-fossati is this a breaking change

This is debatable. Yes, it breaks clients, but these clients are borked too. So, it's more a "fix", or probably a "breaking fix" ;-)

and does it need any change we refer profile in cocli corim submit1 andevcli`

Yes, symmetric changes on the apiclient and cocli side are required.

thomas-fossati commented 3 months ago

Note that end-to-end scripts and the walkthrough (as well as ny other documentation that has examaples of specifying a media type needs to updated as well.

Good point. On the chase.

setrofim commented 3 months ago

@thomas-fossati is this a breaking change

This is debatable. Yes, it breaks clients, but these clients are borked too. So, it's more a "fix", or probably a "breaking fix" ;-)

I belive the clients take the media type as input and handle it opaquely? If so, then this is change to how the clients must be used, and does not break the clients themselves. So not a breaking change.

yogeshbdeshpande commented 3 months ago

@thomas-fossati is this a breaking change

This is debatable. Yes, it breaks clients, but these clients are borked too. So, it's more a "fix", or probably a "breaking fix" ;-)

I belive the clients take the media type as input and handle it opaquely? If so, then this is change to how the clients must be used, and does not break the clients themselves. So not a breaking change.

If any of the existing scripts or how end to end scenarios work gets modified, with the existing commands, it is a breaking change in my view

thomas-fossati commented 3 months ago

I belive the clients take the media type as input and handle it opaquely?

This is the case for cocli, but evcli seems to have a more picky view of the world.

thomas-fossati commented 3 months ago

@thomas-fossati is this a breaking change

This is debatable. Yes, it breaks clients, but these clients are borked too. So, it's more a "fix", or probably a "breaking fix" ;-)

I belive the clients take the media type as input and handle it opaquely? If so, then this is change to how the clients must be used, and does not break the clients themselves. So not a breaking change.

If any of the existing scripts or how end to end scenarios work gets modified, with the existing commands, it is a breaking change in my view

Again, it's breaking something that was already broken, so it's a bit of a grey area.

I'll add amend with an ! just to be sure.

yogeshbdeshpande commented 3 months ago

@thomas-fossati is this a breaking change

This is debatable. Yes, it breaks clients, but these clients are borked too. So, it's more a "fix", or probably a "breaking fix" ;-)

I belive the clients take the media type as input and handle it opaquely? If so, then this is change to how the clients must be used, and does not break the clients themselves. So not a breaking change.

If any of the existing scripts or how end to end scenarios work gets modified, with the existing commands, it is a breaking change in my view

Again, it's breaking something that was already broken, so it's a bit of a grey area.

I'll add amend with an ! just to be sure.

Thanks, I agree that users should be Warn about the change. I just noticed so many outside participants now use Veraison, they should not be suddenly breaking if they take latest of services and things start to fail.

thomas-fossati commented 3 months ago

Thanks for the fix, LGTM, except I would align the commit with the Warning as discussed!

yes, I have amended the commit title already in 5294ee

thomas-fossati commented 3 months ago

@thomas-fossati is this a breaking change

This is debatable. Yes, it breaks clients, but these clients are borked too. So, it's more a "fix", or probably a "breaking fix" ;-)

I belive the clients take the media type as input and handle it opaquely? If so, then this is change to how the clients must be used, and does not break the clients themselves. So not a breaking change.

If any of the existing scripts or how end to end scenarios work gets modified, with the existing commands, it is a breaking change in my view

Again, it's breaking something that was already broken, so it's a bit of a grey area. I'll add amend with an ! just to be sure.

Thanks, I agree that users should be Warn about the change. I just noticed so many outside participants now use Veraison, they should not be suddenly breaking if they take latest of services and things start to fail.

Yes, good point, this is something worth flagging at the next weekly.

yogeshbdeshpande commented 3 months ago

@thomas-fossati is this a breaking change

This is debatable. Yes, it breaks clients, but these clients are borked too. So, it's more a "fix", or probably a "breaking fix" ;-)

I belive the clients take the media type as input and handle it opaquely? If so, then this is change to how the clients must be used, and does not break the clients themselves. So not a breaking change.

If any of the existing scripts or how end to end scenarios work gets modified, with the existing commands, it is a breaking change in my view

Again, it's breaking something that was already broken, so it's a bit of a grey area. I'll add amend with an ! just to be sure.

Thanks, I agree that users should be Warn about the change. I just noticed so many outside participants now use Veraison, they should not be suddenly breaking if they take latest of services and things start to fail.

Yes, good point, this is something worth flagging at the next weekly.

Thanks, is this done deliberately the breaking of line, as below:

`fix!(scheme, api): correctly handle media type parameters with absolu…

…te URIs`

setrofim commented 3 months ago

This happens when your commit title is too long. The title limit is lower than the message body. Can't recall the specific number, but I think it's something like 59 columns.

Maybe change the commit title to

fix!(scheme, api): correct media type params with absolute URIs

thomas-fossati commented 3 months ago

Can't recall the specific number, but I think it's something like 59 columns.

72

setrofim commented 3 months ago

Can't recall the specific number, but I think it's something like 59 columns.

72

Ah, that's it!