Skip to content

Commit 6bf29b7

Browse files
committed
Changed SessionBased authentication to send 401 for sub-requests
Implements #38
1 parent e1b2c55 commit 6bf29b7

3 files changed

Lines changed: 39 additions & 1 deletion

File tree

ChangeLog.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,11 @@ Web Authentication change log
33

44
## ?.?.? / ????-??-??
55

6+
## 7.0.0 / ????-??-??
7+
8+
* Changed `SessionBased` authentication to send 401 for sub-requests (e.g.
9+
images, fetch(), ...), implementing feature suggested in #38
10+
(@thekid)
611
* Merged PR #37: Refactor OAuth1 & OAuth2 flows, fixing possible flow error
712
states and implifiying their implementation
813
(@thekid)

src/main/php/web/auth/SessionBased.class.php

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
/** @test web.auth.unittest.SessionBasedTest */
99
class SessionBased extends Authentication {
1010
const TOKEN_LENGTH= 32;
11+
const IS_NAVIGATION= ['navigate', null];
1112

1213
private static $random;
1314
private $flow, $sessions, $lookup;
@@ -87,6 +88,15 @@ public function filter($req, $res, $invocation) {
8788

8889
if (null === $user) {
8990

91+
// Only start authentication for top-level navigation, issuing 401 errors for
92+
// sub-requests, e.g. to images or when using fetch from JavaScript to prevent
93+
// serving non-sensical authentication redirects to these requests.
94+
if (!in_array($req->header('Sec-Fetch-Mode'), self::IS_NAVIGATION)) {
95+
$res->answer(401);
96+
$res->send('Authentication required', 'text/plain');
97+
return;
98+
}
99+
90100
// Authentication may require redirection in order to fulfill its job.
91101
// In this case, return early from this method w/o passing control on.
92102
if (null === ($result= $this->flow->authenticate($req, $res, $session))) return;

src/test/php/web/auth/unittest/SessionBasedTest.class.php

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
<?php namespace web\auth\unittest;
22

33
use lang\IllegalStateException;
4-
use test\{Assert, Test};
4+
use test\{Assert, Test, Values};
55
use web\auth\{Flow, SessionBased};
66
use web\io\{TestInput, TestOutput};
77
use web\session\{ForTesting, ISession};
@@ -50,9 +50,32 @@ public function redirects_to_sso() {
5050
throw new IllegalStateException('Should not be reached');
5151
}));
5252

53+
Assert::equals(302, $res->status());
5354
Assert::equals('https://sso.example.com/', $res->headers()['Location']);
5455
}
5556

57+
#[Test, Values(['navigate', null])]
58+
public function redirects_for_top_level_requests($mode) {
59+
$auth= new SessionBased($this->authenticate(null), new ForTesting());
60+
$res= $this->handle(['Sec-Fetch-Mode' => $mode], $auth->required(function($req, $res) use(&$user) {
61+
throw new IllegalStateException('Should not be reached');
62+
}));
63+
64+
Assert::equals(302, $res->status());
65+
Assert::equals('https://sso.example.com/', $res->headers()['Location']);
66+
}
67+
68+
#[Test, Values(['cors', 'no-cors', 'same-origin', 'websocket'])]
69+
public function sends_401_for_subrequests($mode) {
70+
$auth= new SessionBased($this->authenticate(null), new ForTesting());
71+
$res= $this->handle(['Sec-Fetch-Mode' => $mode], $auth->required(function($req, $res) use(&$user) {
72+
throw new IllegalStateException('Should not be reached');
73+
}));
74+
75+
Assert::equals(401, $res->status());
76+
Assert::equals('Authentication required', $res->output()->body());
77+
}
78+
5679
#[Test]
5780
public function required() {
5881
$sessions= new ForTesting();

0 commit comments

Comments
 (0)