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

Fix SameSite cookies for Enterprise authentication #231

Merged
merged 3 commits into from
Aug 15, 2022

Conversation

kyushun
Copy link
Contributor

@kyushun kyushun commented Jan 20, 2022

Fix #213

Github Enterprise requires private_mode_user_session for private access, but can't load avatars because SameSite cookie is Lax by default.
https://docs.github.com/en/github/site-policy/github-subprocessors-and-cookies

@treastrain
Copy link

I'm just having trouble with this!
I hope this pull request will be approved early and this app will be better!

await this.mainWindow.webContents.session.cookies.set({
url: `https://${cookie.domain?.replace(/^\./, '')}${cookie.path}`,
secure: cookie.secure,
sameSite: 'no_restriction',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kyushun laxだとうまく動かないでしょうか?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lax は top-level navigation のときのみ cookie を付与するので img タグなどといった URL の変更が行われないリクエストでは無視されます。
ユーザーのアイコンを取得するためのリクエストには private_mode_user_session が必要ですが、デフォルトでは lax になっており送られないので none にして cookie を送るようにして上げる必要があります。

@@ -70,6 +70,36 @@ class _MainWindow {
async initRenderer() {
await this.mainWindow.loadURL(`file://${__dirname}/../../../Renderer/asset/html/index.html`);
// await this.correctCookies();

const privateModeSessionCookies = await this.mainWindow.webContents.session.cookies.get({ name: 'private_mode_user_session' });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

以降のcookieを操作する一連の処理はgithub.com以外のドメインの場合のみ動くようにしたほうが安全になりそうです。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

github.com に private_mode_user_session は存在しないはずですが一応追記しておきました

Copy link
Member

@h13i32maru h13i32maru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kyushun cookieを手動で操作することになるのですが、セキュリティ上の懸念はないでしょうか?

@kyushun
Copy link
Contributor Author

kyushun commented Jul 30, 2022

@h13i32maru

cookieを手動で操作することになるのですが、セキュリティ上の懸念はないでしょうか?

リクエストに private_mode_user_session が送られるようにはなりますが、cookie が指定するドメイン以外には送られることはないので問題ないという認識です。

遅くなりましたが回答・修正しました 🙇‍♂️

@h13i32maru
Copy link
Member

@kyushun 返信ありがとうございます!マージしますね。
僕の手元ではgithub enterpriseの動作確認はできないので、もし何かありましたらメンションさせていただくかもしれないです。その時はよろしくおねがいします。

@h13i32maru h13i32maru merged commit c7c5c05 into jasperapp:master Aug 15, 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

Successfully merging this pull request may close these issues.

Avatars not loading
3 participants