Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Commit

Permalink
Fix #3581, make Screenshots work with third party cookies disabled
Browse files Browse the repository at this point in the history
This adds a second attempt to login to wantsauth logins, one that runs in sitehelper.js, and tries to get the cookie set on a request that appears to come from the content page itself.

Note this does not firmly protect from the content page overwriting window.XMLHttpRequest and having the add-on use that object.
  • Loading branch information
ianb committed Oct 11, 2017
1 parent 76109c2 commit ac75a0b
Show file tree
Hide file tree
Showing 6 changed files with 71 additions and 2 deletions.
9 changes: 8 additions & 1 deletion addon/webextension/background/auth.js
Original file line number Diff line number Diff line change
Expand Up @@ -200,12 +200,19 @@ this.auth = (function() {

communication.register("getAuthInfo", (sender, ownershipCheck) => {
return registrationInfoFetched.then(() => {
return exports.authHeaders();
}).then((authHeaders) => {
let info = registrationInfo;
if (info.registered) {
return login({ownershipCheck}).then((result) => {
return {isOwner: result && result.isOwner, deviceId: registrationInfo.deviceId};
return {
isOwner: result && result.isOwner,
deviceId: registrationInfo.deviceId,
authHeaders
};
});
}
info = Object.assign({authHeaders}, info);
return info;
});
});
Expand Down
31 changes: 31 additions & 0 deletions addon/webextension/sitehelper.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@

this.sitehelper = (function() {

// This gives us the content's copy of XMLHttpRequest, instead of the wrapped
// copy that this content script gets:
let ContentXMLHttpRequest = window.wrappedJSObject.XMLHttpRequest;

catcher.registerHandler((errorObj) => {
callBackground("reportError", errorObj);
});
Expand All @@ -20,13 +24,40 @@ this.sitehelper = (function() {
document.dispatchEvent(new CustomEvent(name, {detail}));
}

/** Set the cookie, even if third-party cookies are disabled in this browser
(when they are disabled, login from the background page won't set cookies) */
function sendBackupCookieRequest(authHeaders) {
// See https://bugzilla.mozilla.org/show_bug.cgi?id=1295660
// This bug would allow us to access window.content.XMLHttpRequest, and get
// a safer (not overridable by content) version of the object.

// This is a very minimal attempt to verify that the XMLHttpRequest object we got
// is legitimate. It is not a good test.
if (Object.toString.apply(ContentXMLHttpRequest) != "function XMLHttpRequest() {\n [native code]\n}") {
console.warn("Insecure copy of XMLHttpRequest");
return;
}
let req = new ContentXMLHttpRequest();
req.open("POST", "/api/set-login-cookie");
for (let name in authHeaders) {
req.setRequestHeader(name, authHeaders[name]);
}
req.send("");
req.onload = () => {
if (req.status != 200) {
console.warn("Attempt to set Screenshots cookie via /api/set-login-cookie failed:", req.status, req.statusText, req.responseText);
}
};
}

document.addEventListener("delete-everything", catcher.watchFunction((event) => {
// FIXME: reset some data in the add-on
}, false));

document.addEventListener("request-login", catcher.watchFunction((event) => {
let shotId = event.detail;
catcher.watchPromise(callBackground("getAuthInfo", shotId || null).then((info) => {
sendBackupCookieRequest(info.authHeaders);
sendCustomEvent("login-successful", {deviceId: info.deviceId, isOwner: info.isOwner});
}));
}));
Expand Down
3 changes: 2 additions & 1 deletion server/src/middleware/csrf.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ function isCsrfExemptPath(path) {
return isAuthPath(path)
|| path.startsWith("/data")
|| path === "/event"
|| path === "/error";
|| path === "/error"
|| path === "/api/set-login-cookie";
}

function csrfHeadersValid(req) {
Expand Down
13 changes: 13 additions & 0 deletions server/src/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -587,6 +587,19 @@ app.post("/api/login", function(req, res) {
});
});

app.post("/api/set-login-cookie", function(req, res) {
if (!req.deviceId) {
sendRavenMessage(req, "Attempt to set login cookie without authentication");
simpleResponse(res, "Not logged in", 401);
return;
}
sendAuthInfo(req, res, {
deviceId: req.deviceId,
accountId: req.accountId,
userAbTests: req.abTests
});
});

app.put("/data/:id/:domain", upload.single('blob'), function(req, res) {
let slowResponse = config.testing.slowResponse;
let failSometimes = config.testing.failSometimes;
Expand Down
1 change: 1 addition & 0 deletions test/server/clientlib.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ def login(self):
urljoin(self.backend, "/api/register"),
data=dict(deviceId=self.deviceId, secret=self.secret, deviceInfo=json.dumps(self.deviceInfo)))
resp.raise_for_status()
return resp

def delete_account(self):
page = self.session.get(self.backend + "/leave-screenshots/").text
Expand Down
16 changes: 16 additions & 0 deletions test/server/test_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,21 @@ def test_login():
assert resp.status_code == 200


def test_set_login_cookie():
sess1 = ScreenshotsClient()
login_resp = sess1.login()
headers = {'x-screenshots-auth': login_resp.json()['authHeader']}
assert sess1.session.cookies['user'] and sess1.session.cookies['user.sig']
sess2 = ScreenshotsClient()
# Trying to login without any authentication won't work:
resp = sess2.session.post(sess2.backend + "/api/set-login-cookie")
assert resp.status_code == 401
assert not sess2.session.cookies.get('user')
resp = sess2.session.post(sess2.backend + "/api/set-login-cookie", headers=headers)
assert resp.status_code == 200
assert sess2.session.cookies['user'] and sess2.session.cookies['user.sig']


if __name__ == "__main__":
test_register_without_deviceid_fails()
test_register_without_secret_fails()
Expand All @@ -181,3 +196,4 @@ def test_login():
test_login_invalid_json_deviceinfo()
test_login_ownership_check()
test_login()
test_set_login_cookie()

0 comments on commit ac75a0b

Please sign in to comment.