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

Possible detection via http2 settings frame order #11

Open
gotsteez opened this issue Sep 7, 2021 · 1 comment
Open

Possible detection via http2 settings frame order #11

gotsteez opened this issue Sep 7, 2021 · 1 comment

Comments

@gotsteez
Copy link
Contributor

gotsteez commented Sep 7, 2021

I overlooked the fact that the http2 settings will technically be ordered with InitialWindowSize and HeaderTableSize always being the last two header frames sent. No site is detecting via http2 setting order at the moment, so it is not problem, but the fix is also pretty easy. In order to fix this error, there needs to be an underlying API change detailed below. What are everyone's opinions on this?

Code that creates error:

	setMaxHeader := false
	if t.Settings != nil {
		for _, setting := range t.Settings {
			if setting.ID == SettingMaxHeaderListSize {
				setMaxHeader = true
			}
			if setting.ID == SettingHeaderTableSize || setting.ID == SettingInitialWindowSize {
				return nil, errSettingsIncludeIllegalSettings
			}
			initialSettings = append(initialSettings, setting)
		}
	}
	if t.InitialWindowSize != 0 {
		initialSettings = append(initialSettings, Setting{ID: SettingInitialWindowSize, Val: t.InitialWindowSize})
	} else {
		initialSettings = append(initialSettings, Setting{ID: SettingInitialWindowSize, Val: transportDefaultStreamFlow})
	}
	if t.HeaderTableSize != 0 {
		initialSettings = append(initialSettings, Setting{ID: SettingHeaderTableSize, Val: t.HeaderTableSize})
	} else {
		initialSettings = append(initialSettings, Setting{ID: SettingHeaderTableSize, Val: initialHeaderTableSize})
	}
	if max := t.maxHeaderListSize(); max != 0 && !setMaxHeader {
		initialSettings = append(initialSettings, Setting{ID: SettingMaxHeaderListSize, Val: max})
	}

Proposed Fix:

  • Have the http2 Transport Settings field changed from
	// Settings should not include InitialWindowSize or HeaderTableSize, set that in Transport
	Settings          []Setting
	InitialWindowSize uint32 // if nil, will use global initialWindowSize
	HeaderTableSize   uint32 // if nil, will use global initialHeaderTableSize

to

    Settings map[SettingID]uint32

This would require everyone using the library to change their code, but since this isn't a pressing issue that seems like unnecessary work.

bogdanfinn pushed a commit to bogdanfinn/fhttp that referenced this issue Apr 21, 2022
@bogdanfinn
Copy link

@zMrKrabz i created a branch on my fork adressing this issue and taking your proposed fix into account.

my issue was next to the order issue that my fhttp client always sends MAX_FRAME_SIZE and ENABLE_PUSH. My browser did not do it actually. therefore i need another possibility to define the settings which are actually sent next to the order of the settings.

ranging over Settings map[SettingID]uint32 might fuck up the order because of the random iteration over a map. but more to this can be found in my changeset.

feedback is appreciated :-)

bogdanfinn pushed a commit to bogdanfinn/fhttp that referenced this issue Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants