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.

Sign up for GitHub

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

Jump to bottom

Enhance header name validation exception message (approach 1) #2923

Closed
wants to merge 1 commit into from
Closed

Enhance header name validation exception message (approach 1) #2923

wants to merge 1 commit into from

Conversation

idelpivnitskiy
Copy link
Member

Motivation:

Provide more details about a illegal header name.

Modifications:

  • Try-catch IllegalCharacterException in HeaderUtils and rewrap in StacklessIllegalArgumentException with additional context (token value);

Result:

Users can see what header name has an illegal character.

This approach optimizes for a "happy path".

When users add/set illegal header name:

before

Exception in thread "main" io.servicetalk.utils.internal.IllegalCharacterException: '�' (0x00), expected [! / # / $ / % / & / ' / * / + / - / . / ^ / _ / ` / | / ~ / DIGIT / ALPHA]
	at io.servicetalk.http.api.HeaderUtils.validateTokenChar(HeaderUtils.java:831)
	at io.servicetalk.buffer.api.CharSequences.forEachByte(CharSequences.java:126)
	at io.servicetalk.http.api.HeaderUtils.validateToken(HeaderUtils.java:243)
	at io.servicetalk.http.api.DefaultHttpHeaders.validateHeaderName(DefaultHttpHeaders.java:591)
	at io.servicetalk.http.api.DefaultHttpHeaders.validateKey(DefaultHttpHeaders.java:572)
	at io.servicetalk.http.api.DefaultHttpHeaders.validateKey(DefaultHttpHeaders.java:45)
	at io.servicetalk.http.api.MultiMap.putExclusive(MultiMap.java:272)
	at io.servicetalk.http.api.DefaultHttpHeaders.set(DefaultHttpHeaders.java:672)
	at io.servicetalk.http.api.HttpMetaData.setHeader(HttpMetaData.java:126)
	at io.servicetalk.http.api.HttpRequestMetaData.setHeader(HttpRequestMetaData.java:486)
	at io.servicetalk.http.api.StreamingHttpRequest.setHeader(StreamingHttpRequest.java:324)
	at io.servicetalk.http.api.DefaultHttpRequest.setHeader(DefaultHttpRequest.java:168)
	at io.servicetalk.examples.http.helloworld.blocking.BlockingHelloWorldClient.main(BlockingHelloWorldClient.java:32)

after

Exception in thread "main" io.servicetalk.http.api.HeaderUtils$StacklessIllegalArgumentException: Invalid header name: header-�-name
Caused by: io.servicetalk.utils.internal.IllegalCharacterException: '�' (0x00), expected [! / # / $ / % / & / ' / * / + / - / . / ^ / _ / ` / | / ~ / DIGIT / ALPHA]
	at io.servicetalk.http.api.HeaderUtils.validateTokenChar(HeaderUtils.java:838)
	at io.servicetalk.buffer.api.CharSequences.forEachByte(CharSequences.java:126)
	at io.servicetalk.http.api.HeaderUtils.validateToken(HeaderUtils.java:245)
	at io.servicetalk.http.api.DefaultHttpHeaders.validateHeaderName(DefaultHttpHeaders.java:591)
	at io.servicetalk.http.api.DefaultHttpHeaders.validateKey(DefaultHttpHeaders.java:572)
	at io.servicetalk.http.api.DefaultHttpHeaders.validateKey(DefaultHttpHeaders.java:45)
	at io.servicetalk.http.api.MultiMap.putExclusive(MultiMap.java:272)
	at io.servicetalk.http.api.DefaultHttpHeaders.set(DefaultHttpHeaders.java:672)
	at io.servicetalk.http.api.HttpMetaData.setHeader(HttpMetaData.java:126)
	at io.servicetalk.http.api.HttpRequestMetaData.setHeader(HttpRequestMetaData.java:486)
	at io.servicetalk.http.api.StreamingHttpRequest.setHeader(StreamingHttpRequest.java:324)
	at io.servicetalk.http.api.DefaultHttpRequest.setHeader(DefaultHttpRequest.java:168)
	at io.servicetalk.examples.http.helloworld.blocking.BlockingHelloWorldClient.main(BlockingHelloWorldClient.java:34)

When an illegal header name is received over network:

before

2024-05-13 13:51:24,884 servicetalk-global-io-executor-1-2 [WARN ] NettyHttpServer$ErrorLoggingHttpSubscriber - [id: 0xeb527ce7, L:/127.0.0.1:8080 - R:/127.0.0.1:51347] Can not decode a message, no more requests will be received on this HTTP/1.1 connection, closing it due to:
io.servicetalk.http.netty.HttpObjectDecoder$StacklessDecoderException: Invalid header name in line 2: header-�-name
Caused by: io.servicetalk.utils.internal.IllegalCharacterException: '�' (0x00), expected [! / # / $ / % / & / ' / * / + / - / . / ^ / _ / ` / | / ~ / DIGIT / ALPHA]
	at io.servicetalk.http.api.HeaderUtils.validateTokenChar(HeaderUtils.java:831) ~[servicetalk-http-api/:?]
	at io.netty.buffer.AbstractByteBuf.forEachByteAsc0(AbstractByteBuf.java:1301) ~[netty-buffer-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.buffer.AbstractByteBuf.forEachByte(AbstractByteBuf.java:1281) ~[netty-buffer-4.1.109.Final.jar:4.1.109.Final]
	at io.servicetalk.buffer.netty.NettyBuffer.forEachByte(NettyBuffer.java:840) ~[servicetalk-buffer-netty/:?]
	at io.servicetalk.buffer.netty.WrappedBuffer.forEachByte(WrappedBuffer.java:784) ~[servicetalk-buffer-netty/:?]
	at io.servicetalk.buffer.api.AsciiBuffer.forEachByte(AsciiBuffer.java:132) ~[servicetalk-buffer-api/:?]
	at io.servicetalk.buffer.api.CharSequences.forEachByte(CharSequences.java:123) ~[servicetalk-buffer-api/:?]
	at io.servicetalk.http.api.HeaderUtils.validateToken(HeaderUtils.java:243) ~[servicetalk-http-api/:?]
	at io.servicetalk.http.api.DefaultHttpHeaders.validateHeaderName(DefaultHttpHeaders.java:591) ~[servicetalk-http-api/:?]
	at io.servicetalk.http.api.DefaultHttpHeaders.validateKey(DefaultHttpHeaders.java:572) ~[servicetalk-http-api/:?]
	at io.servicetalk.http.api.DefaultHttpHeaders.validateKey(DefaultHttpHeaders.java:45) ~[servicetalk-http-api/:?]
	at io.servicetalk.http.api.MultiMap.put(MultiMap.java:226) ~[servicetalk-http-api/:?]
	at io.servicetalk.http.api.DefaultHttpHeaders.add(DefaultHttpHeaders.java:638) ~[servicetalk-http-api/:?]
	at io.servicetalk.http.netty.HttpObjectDecoder.parseHeaderLine(HttpObjectDecoder.java:687) ~[servicetalk-http-netty/:?]
	at io.servicetalk.http.netty.HttpObjectDecoder.parseAllHeaders(HttpObjectDecoder.java:788) ~[servicetalk-http-netty/:?]
	at io.servicetalk.http.netty.HttpObjectDecoder.readHeaders(HttpObjectDecoder.java:718) ~[servicetalk-http-netty/:?]
	at io.servicetalk.http.netty.HttpObjectDecoder.decode(HttpObjectDecoder.java:299) ~[servicetalk-http-netty/:?]
	at io.servicetalk.transport.netty.internal.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:365) ~[servicetalk-transport-netty-internal/:?]
	at io.servicetalk.transport.netty.internal.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:321) ~[servicetalk-transport-netty-internal/:?]
	at io.servicetalk.transport.netty.internal.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:185) ~[servicetalk-transport-netty-internal/:?]
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:444) [netty-transport-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420) [netty-transport-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412) [netty-transport-4.1.109.Final.jar:4.1.109.Final]
	at io.servicetalk.transport.netty.internal.CopyByteBufHandlerChannelInitializer$CopyByteBufHandler.channelRead(CopyByteBufHandlerChannelInitializer.java:98) [servicetalk-transport-netty-internal/:?]
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:444) [netty-transport-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420) [netty-transport-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412) [netty-transport-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410) [netty-transport-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:440) [netty-transport-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420) [netty-transport-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919) [netty-transport-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.channel.kqueue.AbstractKQueueStreamChannel$KQueueStreamUnsafe.readReady(AbstractKQueueStreamChannel.java:544) [netty-transport-classes-kqueue-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.channel.kqueue.AbstractKQueueChannel$AbstractKQueueUnsafe.readReady(AbstractKQueueChannel.java:387) [netty-transport-classes-kqueue-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.channel.kqueue.KQueueEventLoop.processReady(KQueueEventLoop.java:218) [netty-transport-classes-kqueue-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.channel.kqueue.KQueueEventLoop.run(KQueueEventLoop.java:296) [netty-transport-classes-kqueue-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997) [netty-common-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74) [netty-common-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) [netty-common-4.1.109.Final.jar:4.1.109.Final]
	at java.base/java.lang.Thread.run(Thread.java:829) [?:?]

after

2024-05-13 13:49:52,053 servicetalk-global-io-executor-1-2 [WARN ] NettyHttpServer$ErrorLoggingHttpSubscriber - [id: 0x77de9fb3, L:/127.0.0.1:8080 - R:/127.0.0.1:51299] Can not decode a message, no more requests will be received on this HTTP/1.1 connection, closing it due to:
io.servicetalk.http.netty.HttpObjectDecoder$StacklessDecoderException: Invalid header name in line 2: header-�-name
Caused by: io.servicetalk.http.api.HeaderUtils$StacklessIllegalArgumentException: Invalid header name: header-�-name
Caused by: io.servicetalk.utils.internal.IllegalCharacterException: '�' (0x00), expected [! / # / $ / % / & / ' / * / + / - / . / ^ / _ / ` / | / ~ / DIGIT / ALPHA]
	at io.servicetalk.http.api.HeaderUtils.validateTokenChar(HeaderUtils.java:838) ~[servicetalk-http-api/:?]
	at io.netty.buffer.AbstractByteBuf.forEachByteAsc0(AbstractByteBuf.java:1301) ~[netty-buffer-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.buffer.AbstractByteBuf.forEachByte(AbstractByteBuf.java:1281) ~[netty-buffer-4.1.109.Final.jar:4.1.109.Final]
	at io.servicetalk.buffer.netty.NettyBuffer.forEachByte(NettyBuffer.java:840) ~[servicetalk-buffer-netty/:?]
	at io.servicetalk.buffer.netty.WrappedBuffer.forEachByte(WrappedBuffer.java:784) ~[servicetalk-buffer-netty/:?]
	at io.servicetalk.buffer.api.AsciiBuffer.forEachByte(AsciiBuffer.java:132) ~[servicetalk-buffer-api/:?]
	at io.servicetalk.buffer.api.CharSequences.forEachByte(CharSequences.java:123) ~[servicetalk-buffer-api/:?]
	at io.servicetalk.http.api.HeaderUtils.validateToken(HeaderUtils.java:245) ~[servicetalk-http-api/:?]
	at io.servicetalk.http.api.DefaultHttpHeaders.validateHeaderName(DefaultHttpHeaders.java:591) ~[servicetalk-http-api/:?]
	at io.servicetalk.http.api.DefaultHttpHeaders.validateKey(DefaultHttpHeaders.java:572) ~[servicetalk-http-api/:?]
	at io.servicetalk.http.api.DefaultHttpHeaders.validateKey(DefaultHttpHeaders.java:45) ~[servicetalk-http-api/:?]
	at io.servicetalk.http.api.MultiMap.put(MultiMap.java:226) ~[servicetalk-http-api/:?]
	at io.servicetalk.http.api.DefaultHttpHeaders.add(DefaultHttpHeaders.java:638) ~[servicetalk-http-api/:?]
	at io.servicetalk.http.netty.HttpObjectDecoder.parseHeaderLine(HttpObjectDecoder.java:687) ~[servicetalk-http-netty/:?]
	at io.servicetalk.http.netty.HttpObjectDecoder.parseAllHeaders(HttpObjectDecoder.java:788) ~[servicetalk-http-netty/:?]
	at io.servicetalk.http.netty.HttpObjectDecoder.readHeaders(HttpObjectDecoder.java:718) ~[servicetalk-http-netty/:?]
	at io.servicetalk.http.netty.HttpObjectDecoder.decode(HttpObjectDecoder.java:299) ~[servicetalk-http-netty/:?]
	at io.servicetalk.transport.netty.internal.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:365) ~[servicetalk-transport-netty-internal/:?]
	at io.servicetalk.transport.netty.internal.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:321) ~[servicetalk-transport-netty-internal/:?]
	at io.servicetalk.transport.netty.internal.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:185) ~[servicetalk-transport-netty-internal/:?]
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:444) [netty-transport-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420) [netty-transport-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412) [netty-transport-4.1.109.Final.jar:4.1.109.Final]
	at io.servicetalk.transport.netty.internal.CopyByteBufHandlerChannelInitializer$CopyByteBufHandler.channelRead(CopyByteBufHandlerChannelInitializer.java:98) [servicetalk-transport-netty-internal/:?]
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:444) [netty-transport-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420) [netty-transport-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412) [netty-transport-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410) [netty-transport-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:440) [netty-transport-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420) [netty-transport-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919) [netty-transport-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.channel.kqueue.AbstractKQueueStreamChannel$KQueueStreamUnsafe.readReady(AbstractKQueueStreamChannel.java:544) [netty-transport-classes-kqueue-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.channel.kqueue.AbstractKQueueChannel$AbstractKQueueUnsafe.readReady(AbstractKQueueChannel.java:387) [netty-transport-classes-kqueue-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.channel.kqueue.KQueueEventLoop.processReady(KQueueEventLoop.java:218) [netty-transport-classes-kqueue-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.channel.kqueue.KQueueEventLoop.run(KQueueEventLoop.java:296) [netty-transport-classes-kqueue-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997) [netty-common-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74) [netty-common-4.1.109.Final.jar:4.1.109.Final]
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) [netty-common-4.1.109.Final.jar:4.1.109.Final]
	at java.base/java.lang.Thread.run(Thread.java:829) [?:?]

@idelpivnitskiy
Enhance header name validation exception message
43ecfcc 
Motivation:

Provide more details about a illegal header name.

Modifications:

- Try-catch `IllegalCharacterException` in `HeaderUtils` and rewrap in
`StacklessIllegalArgumentException` with additional context (token
value);

Result:

Users can see what header name has an illegal character.
@idelpivnitskiy idelpivnitskiy self-assigned this May 13, 2024
@idelpivnitskiy
Copy link
Member Author

Lmk what approach you like more and I will adjust tests as needed.

@idelpivnitskiy idelpivnitskiy changed the title Enhance header name validation exception message Enhance header name validation exception message (approach 1) May 13, 2024
@idelpivnitskiy
Copy link
Member Author

Closing in favor of #2924

@idelpivnitskiy idelpivnitskiy deleted the headerName branch May 20, 2024 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bryce-anderson bryce-anderson Awaiting requested review from bryce-anderson

@tkountis tkountis Awaiting requested review from tkountis

Assignees

@idelpivnitskiy idelpivnitskiy

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
1 participant
@idelpivnitskiy