Skip to content

Commit 6d0d2f6

Browse files
committed
fix(logout): match legacy redirect target and 302 status
The legacy Ruby logout redirects via redirect_continue(continue || "/"), issuing a 302 to the continue target when safe, to "/" when no continue is given, and to the authority logout_url only as the cross-domain fallback. auth.cr issued a 303 and defaulted to the authority logout_url even with no continue. Align the target selection and use 302 so logout behaves as a drop-in for existing clients.
1 parent e374f43 commit 6d0d2f6

2 files changed

Lines changed: 36 additions & 6 deletions

File tree

spec/controllers/sessions_spec.cr

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,10 @@ module PlaceOS::Auth
109109
end
110110

111111
describe "GET /auth/logout" do
112-
it "clears the session, stamps logged_out_at, and redirects" do
112+
# The legacy Ruby service redirects logout via `redirect_continue`,
113+
# which issues a 302 (Rails default) to `continue || "/"`, using the
114+
# authority's logout_url only as the cross-domain fallback.
115+
it "clears the session, stamps logged_out_at, and 302-redirects to /" do
113116
password = "ok-password-1234"
114117
user = create_user.call(password)
115118
cookie = Spec.signin!(client, user, password)
@@ -119,7 +122,8 @@ module PlaceOS::Auth
119122
"Host" => "localhost",
120123
"Cookie" => cookie,
121124
})
122-
result.status_code.should eq 303
125+
result.status_code.should eq 302
126+
result.headers["Location"].should eq "/"
123127

124128
reloaded = ::PlaceOS::Model::User.find!(user.id.as(String))
125129
reloaded.logged_out_at.should_not be_nil
@@ -128,9 +132,29 @@ module PlaceOS::Auth
128132
user.try &.destroy
129133
end
130134

131-
it "redirects safely even without a session" do
135+
it "302-redirects to / even without a session" do
132136
result = client.get("/auth/logout", headers: HTTP::Headers{"Host" => "localhost"})
133-
result.status_code.should eq 303
137+
result.status_code.should eq 302
138+
result.headers["Location"].should eq "/"
139+
end
140+
141+
it "redirects to a safe same-site `continue` target" do
142+
result = client.get("/auth/logout?continue=%2Fbye", headers: HTTP::Headers{"Host" => "localhost"})
143+
result.status_code.should eq 302
144+
result.headers["Location"].should eq "/bye"
145+
end
146+
147+
it "falls back to the authority logout_url for a cross-domain `continue`" do
148+
authority = ::PlaceOS::Model::Authority.find_by_domain("localhost").not_nil!
149+
authority.logout_url = "/signed-out"
150+
authority.save!
151+
152+
result = client.get(
153+
"/auth/logout?continue=https%3A%2F%2Fevil.example%2Fx",
154+
headers: HTTP::Headers{"Host" => "localhost"},
155+
)
156+
result.status_code.should eq 302
157+
result.headers["Location"].should eq "/signed-out"
134158
end
135159
end
136160

src/placeos-auth/controllers/sessions.cr

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -113,12 +113,18 @@ module PlaceOS::Auth
113113
end
114114
end
115115

116-
target = if continue && (safe = sanitize_continue(continue))
116+
# Mirror the legacy `redirect_continue(continue || "/")`: default to
117+
# "/", honour a safe same-site continue, and only fall back to the
118+
# authority's logout_url when continue points at another domain.
119+
# Rails' `redirect_to` defaults to 302, so match that status too.
120+
target = if continue.nil?
121+
"/"
122+
elsif (safe = sanitize_continue(continue))
117123
safe
118124
else
119125
authority_logout_target(authority)
120126
end
121-
redirect_to target.gsub(' ', "%20"), :see_other
127+
redirect_to target.gsub(' ', "%20"), :found
122128
end
123129

124130
# ---------- GET /auth/login ------------------------------------------

0 commit comments

Comments
 (0)