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

PHP: reject metadata keys that are not legal #7896

Merged
merged 2 commits into from
Aug 29, 2016

Conversation

stanley-cheung
Copy link
Contributor

Fixes #7895

@stanley-cheung stanley-cheung self-assigned this Aug 26, 2016
@stanley-cheung stanley-cheung force-pushed the php-fix-per-rpc-creds-v1_0 branch 2 times, most recently from 55f9eb7 to ac99f26 Compare August 27, 2016 02:52
@stanley-cheung stanley-cheung changed the title PHP: fix per_rpc_creds capital auth header key PHP: reject metadata keys that are not legal Aug 29, 2016
@stanley-cheung
Copy link
Contributor Author

stanley-cheung commented Aug 29, 2016

Now the C extension rejects any metadata key that fails the grpc_header_key_is_legal check. For the normal code path, the return false case has already been handled. For the plugin_get_metadata callback, an error code is now being returned to core via the grpc_credentials_plugin_metadata_cb callback.

Since it's now the caller's responsibility to make sure metadata keys are lower case, I keep the strtolower conversion in the per_rpc_creds interop tests.

PTAL again, thanks!

@murgatroid99
Copy link
Member

I don't think it needs to be in this change, but now that you're validating keys, it would probably also be a good idea to validate metadata values.

Otherwise, LGTM

@stanley-cheung stanley-cheung merged commit e1ec9fd into grpc:v1.0.x Aug 29, 2016
@stanley-cheung stanley-cheung deleted the php-fix-per-rpc-creds-v1_0 branch August 29, 2016 18:48
@lock lock bot locked as resolved and limited conversation to collaborators Jan 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants