Skip to content
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

Full resolver source is leaked in error message when no subscription topics are provided #489

Closed
bfmiv opened this issue Dec 12, 2019 · 5 comments
Assignees
Labels
Bug 🐛 Something isn't working Community 👨‍👧 Something initiated by a community Solved ✔️ The issue has been solved
Milestone

Comments

@bfmiv
Copy link

bfmiv commented Dec 12, 2019

Describe the bug
Full resolver source is leaked in error message when no subscription topics are provided. This appears to be caused by this line and should be easy to fix with either a toString method or changing the target reference to target.name.

super(`${target}#${methodName} subscription has no provided topics!`);

To Reproduce
Given the resolver:

@Resolver(() => Test)
export default class TestResolver {
    @Subscription(() => Something, {
        topics: () => []
    })
    public watchSomething(@Root() payload: Something): Something {
        return payload;
    }
}

Attempting to subscribe to watchSomething with:

subscription Test {
    watchSomething {
        id
    }
}

Returns the following:

{
    "errors": [
        {
            "message": "class TestResolver {\n    watchSomething(payload) {\n        return payload;\n    }\n}#watchSomething subscription has no provided topics!",
            "locations": [
                {
                    "line": 2,
                    "column": 3
                }
            ],
            "path": [
                "watchSomething"
            ],
            "extensions": {
                "code": "INTERNAL_SERVER_ERROR"
            }
        }
    ]
}

Expected behavior
Only the name of the resolver should be logged

Logs
N/A

Enviorment (please complete the following information):

Additional context
Add any other context about the problem here.

@bfmiv bfmiv changed the title Full resolver source is leaked in error message when no subscription are provided Full resolver source is leaked in error message when no subscription topics are provided Dec 12, 2019
@bfmiv
Copy link
Author

bfmiv commented Dec 12, 2019

@MichalLytek I didn't realize until after I submitted topics can be passed in user args - please take a look at this ASAP

@bfmiv
Copy link
Author

bfmiv commented Dec 12, 2019

@MichalLytek 911 🚨

Sorry to nag but this is a big deal and needs to be patched immediately

@MichalLytek MichalLytek added Bug 🐛 Something isn't working Community 👨‍👧 Something initiated by a community labels Dec 13, 2019
@MichalLytek
Copy link
Owner

MichalLytek commented Dec 13, 2019

this is a big deal

The big deal is that you don't filter the errors returned to the client. This way you can also return failed SQL query or a full stacktrace from internal server error.

The ${target} is obviously a bug but the error but the MissingSubscriptionTopicsError is only a developer error to notify him when forgot to pass the topics, for convinience, just like InterfaceResolveTypeError or UnionResolveTypeError. It's not intended to be an info for the client and should be stripped away from the output.

I submitted topics can be passed in user args

Always validate and sanitize your inputs, don't trust users. So you should check the arg and accept only a set of values for topics, as well as check the length, don't just pass the input as topic to subscription.

And I think that reporting it to npm won't help and won't speed up things. If you need a quick fix, checkout v0.17.5 tag, apply the fix and publish a fork on npm. It's OSS, not a paid software with a warranty.

@MichalLytek MichalLytek added this to the 1.0.0 release milestone Dec 13, 2019
@bfmiv
Copy link
Author

bfmiv commented Dec 13, 2019

Right, I get that. The reason I say it's a big deal has more to do with the fact that I opened a public github issue for what ended up being a potentially devastating exploit (sorry!). But if you're not worried about it 🤷‍♂

Great project btw, thanks for all of your work on this! I will update my PR to address your comments.

@MichalLytek
Copy link
Owner

Fixed via 26ee0ce, released as v0.17.6 🔒

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🐛 Something isn't working Community 👨‍👧 Something initiated by a community Solved ✔️ The issue has been solved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants