Repository navigation
fix(security): reject unknown and unsupported security schemes (#1563) - #1589
Open
bhushan4work wants to merge 1 commit into
Open
bhushan4work wants to merge 1 commit into
bhushan4work wants to merge 1 commit into
Conversation
…se-thingweb#1563) - Explicitly fail on unknown or unsupported security schemes in client and server paths rather than downgrading to nosec/anonymous or warning-only behavior - Preserve valid cross-binding behavior by recognizing and ignoring foreign-binding schemes where appropriate - Support multi-scheme metadata in HttpClient.setSecurity and prevent HttpServer.fillSecurityScheme from wiping out foreign scheme definitions - Add regression tests covering unknown prefixed schemes, unprefixed validation behavior, foreign-binding schemes, and exposure behavior
bhushan4work
requested review from
JKRhb,
danielpeintner and
relu91
as code owners
October 28, 2026 11:07
Contributor
Author
|
i investigated the failed checks. the windows/node 22 job failed due to a network timeout. the macos/node 22 job passed installation and build but was cancelled. windows/node 24 and ubuntu/node 22 passed. I think no code changes is necessary atp. could you please rerun the failed checks? |
Contributor
Author
|
@danielpeintner is any further fix needed in this pr before the final merge ? |
relu91
requested changes
Nov 5, 2026
relu91
left a comment
relu91
left a comment
Member
There was a problem hiding this comment.
Just an intial comment that would probably imply more changes in the overall proposal. Reading #1563 my initial idea was that it should be solved at TD validation level, The TD schema has two levels of verification:
- the scheme is one of the standard schemes
- the scheme is prefixed
Since node-wot currently does not allow any schema extension, we should simply don't accept a TD with a schema prefixed here
Comment on lines
+267
to
-318
| for (const security of metadata) { | ||
| switch (security.scheme) { | ||
| case "basic": { | ||
| const securityBasic: BasicSecurityScheme = <BasicSecurityScheme>security; | ||
|
|
||
| if (!hasSetCredential) { | ||
| this.credential = new BasicCredential( | ||
| credentials as BasicCredentialConfiguration, | ||
| securityBasic | ||
| ); | ||
| hasSetCredential = true; | ||
| } | ||
| break; | ||
| } | ||
| case "bearer": { | ||
| const securityBearer: BearerSecurityScheme = <BearerSecurityScheme>security; | ||
|
|
||
| if (!hasSetCredential) { | ||
| this.credential = new BearerCredential( | ||
| credentials as BearerCredentialConfiguration, | ||
| securityBearer | ||
| ); | ||
| hasSetCredential = true; | ||
| } | ||
| break; | ||
| } | ||
| case "apikey": { | ||
| const securityAPIKey: APIKeySecurityScheme = <APIKeySecurityScheme>security; | ||
|
|
||
| if (!hasSetCredential) { | ||
| this.credential = new BasicKeyCredential( | ||
| credentials as BasicKeyCredentialConfiguration, | ||
| securityAPIKey | ||
| ); | ||
| hasSetCredential = true; | ||
| } | ||
| break; | ||
| } | ||
| case "oauth2": { | ||
| const securityOAuth: OAuth2SecurityScheme = <OAuth2SecurityScheme>security; | ||
|
|
||
| if (!hasSetCredential) { | ||
| if (securityOAuth.flow === "client") { | ||
| securityOAuth.flow = "client_credentials"; | ||
| this.credential = this.oauth.handleClient( | ||
| securityOAuth, | ||
| credentials as OAuthClientConfiguration | ||
| ); | ||
| } else if (securityOAuth.flow === "password") { | ||
| this.credential = this.oauth.handleResourceOwnerCredential( | ||
| securityOAuth, | ||
| credentials as OAuthResourceOwnerConfiguration | ||
| ); | ||
| } | ||
| hasSetCredential = true; | ||
| } | ||
| break; | ||
| } | ||
| case "TuyaCustomBearer": { | ||
| if (!hasSetCredential) { | ||
| this.credential = new TuyaCustomBearer( | ||
| credentials as TuyaCustomBearerCredentialConfiguration, | ||
| security as TuyaCustomBearerSecurityScheme | ||
| ); | ||
| hasSetCredential = true; | ||
| } | ||
| break; | ||
| } | ||
| case "nosec": | ||
| break; | ||
| default: { | ||
| // Schemes belonging to other bindings must remain ignored where appropriate. | ||
| // An unknown or unsupported scheme that is not owned by another binding must fail explicitly. | ||
| if (this.isForeignSecurityScheme(security.scheme)) { | ||
| break; | ||
| } | ||
| throw new Error(`HttpClient cannot set security scheme '${security.scheme}'`); | ||
| } | ||
|
|
||
| break; | ||
| } | ||
| case "TuyaCustomBearer": { | ||
| this.credential = new TuyaCustomBearer( | ||
| credentials as TuyaCustomBearerCredentialConfiguration, | ||
| security as TuyaCustomBearerSecurityScheme | ||
| ); | ||
| break; | ||
| } | ||
| case "nosec": | ||
| break; | ||
| default: | ||
| error(`HttpClient cannot set security scheme '${security.scheme}'. ${metadata}`); | ||
| return false; | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
I would simplify the logic here. Given the following:
- TD must have at least one security scheme active (https://www.w3.org/TR/wot-thing-description11/#td-security-mandatory)
- And the metadata array can be: "one security scheme" or "multiple security schemes" -> but they should be treated as a ComboSecurityScheme (see https://www.w3.org/TR/wot-thing-description11/#td-security-combo-deprecation).
To properly fix #1563 I would say that:
- client finds a security scheme it supports (this includes no_sec too)
- or it doesn't -> throw
Moreover, since the metadata array is computed here and given the previously mentioned assertion if the array is longer than one -> it is an unsupported scheme (basically is a ComboScheme).
@danielpeintner and @egekorkan correct me if I'm wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed and why
nosecor anonymous authentication.HttpServer.fillSecuritySchemefrom removing foreign security definitions.Checks
npm run buildpassednpm test -w packages/binding-http92 passingnpm test -w packages/binding-opcua98 passing, 1 pendingnpm test -w packages/core145 passing, 1 pendingnpm run lintpassednpx prettier --checkpassed