-
Notifications
You must be signed in to change notification settings - Fork 147
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
Authenticate requests to the /provision endpoint #1035
Conversation
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.
Two minor issues, but otherwise this looks good.
When there are many projects already using matrix-appservice-bridge, why don't we release a types/matrix-appservice-bridge
package instead of keeping the types updated locally?
src/provisioning/Provisioner.ts
Outdated
if (this.isProvisionRequest(req)) { | ||
res.header("Access-Control-Allow-Origin", "*"); | ||
res.header("Access-Control-Allow-Headers", | ||
"Origin, X-Requested-With, Content-Type, Accept"); | ||
} | ||
next(); | ||
return next(); |
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.
What does returning next()
do? I've never seen that as a pattern.
src/provisioning/Provisioner.ts
Outdated
if (!this.ircBridge.getAppServiceBridge().requestCheckToken(req)) { | ||
return res.status(403).send({ | ||
errcode: "M_FORBIDDEN", | ||
error: "Bad token supplied," |
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.
error: "Bad token supplied," | |
error: "Bad token supplied" |
src/provisioning/Provisioner.ts
Outdated
@@ -132,12 +132,18 @@ export class Provisioner { | |||
|
|||
// Deal with CORS (temporarily for s-web) |
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.
This comment which once described the entire purpose of this handler has now become outdated.
We should probably just bundle a definition with the library itself. I've not done it yet due to time |
Hey, since this is marked as |
We're not treating it as high enough severity to warrant a CVE (which is why it didn't get a minor release and has been tacked onto the next release). It only affects users who enabled provisioning and didn't firewall the application out from the rest of the internet. We've determined this to be of low severity because the provisioning system is useless unless you are also running an integration manager (and the feature is off by default), and you'd have to be exposing your appservice to the world. |
Hm, I personally I feel like even for low severity security issues a CVE should be issued, otherwise the chance for people who happen to run the appservice in that configuration (for whatever reason), won't know about that security issue. |
No description provided.