close the review of the Kepr remote form: validation, no shell, no redirects (3.0.96)

@@ -1 +1 @@1-3.0.951+3.0.96

@@ -22,6 +22,9 @@2222 		values: ['code']2323 		flags:  ['json', 'forget']2424 	}) or { return usage_error(err.msg()) }25+	if args.value('code') != '' && args.positional.len == 0 {26+		return usage_error('--code belongs to "koh remote <host>"')27+	}2528 	if args.flag('forget') {2629 		if !machine.has_remote() {2730 			return refuse(args.json(), 'this machine has no remote to forget')@@ -82,6 +85,9 @@8285 	if args.positional_at(0) !in [machine.kind_folder, machine.kind_s3] {8386 		return set_kepr_remote(args)8487 	}88+	if args.value('code') != '' {89+		return usage_error('--code belongs to "koh remote <host>", not to folder or s3')90+	}8591 	kind := args.positional_at(0)8692 	match kind {8793 		machine.kind_folder {@@ -117,52 +123,95 @@117123 	}118124 }119125 126+// kepr_host normalises a Kepr address: https unless the scheme says otherwise, and scheme, host127+// and port only. A path, a query, a fragment or credentials would send the code somewhere else.128+fn kepr_host(value string) !string {129+	for letter in value {130+		if letter <= ` ` {131+			return error('"${value}" is not a Kepr address — try kepr.uk')132+		}133+	}134+	text := if value.starts_with('http://') || value.starts_with('https://') {135+		value136+	} else {137+		'https://${value}'138+	}139+	parsed := urllib.parse(text) or { return error('"${value}" is not a Kepr address — try kepr.uk') }140+	if parsed.scheme !in ['http', 'https'] || parsed.host == '' || parsed.path.trim_right('/') != ''141+		|| parsed.raw_query != '' || parsed.fragment != '' {142+		return error('"${value}" is not a Kepr address — try kepr.uk')143+	}144+	// Credentials in the address have nowhere to go: `urllib` would quietly drop them, and Kepr145+	// would see a different address than the one typed.146+	if text.all_after('//').contains('@') {147+		return error('"${value}" is not a Kepr address — try kepr.uk')148+	}149+	return '${parsed.scheme}://${parsed.host}'150+}151+120152 fn set_kepr_remote(args Args) int {121153 	if args.positional.len != 1 {122154 		return usage_error('koh remote <host> accepts one Kepr host')123155 	}124-	mut host := args.positional_at(0)125-	if !host.starts_with('http://') && !host.starts_with('https://') {126-		host = 'https://${host}'127-	}128-	host = host.trim_right('/')129-	connect_url := '${host}/connect?machine=${machine.name()}'156+	host := kepr_host(args.positional_at(0)) or { return refuse(args.json(), err.msg()) }157+	connect_url := '${host}/connect?machine=${urllib.query_escape(machine.name())}'130158 	println('Open ${connect_url} and sign in')131-	open_command := if os.user_os() == 'macos' { 'open' } else { 'xdg-open' }132-	// Quoted: a machine name can hold anything, and this line goes to the shell.133-	os.execute('${open_command} "${connect_url}" >/dev/null 2>&1 &')134159 	mut code := args.value('code')135160 	if code == '' {136161 		if !interactive() {137162 			return fail('setting up a Kepr remote needs a terminal, or use --code <code>')138163 		}164+		// Only a person about to type the code gets a browser opened for them: with `--code` the165+		// page has been read already, and a script must never take over the desktop. `open` or166+		// `xdg-open` is launched with an argument vector rather than a shell — neither the host nor167+		// this machine's name is ours to quote for one — and a browser that will not open is not168+		// an error.169+		mut browser := os.new_process(if os.user_os() == 'macos' { 'open' } else { 'xdg-open' })170+		browser.set_args([connect_url])171+		browser.run()139172 		print('paste the code: ')140173 		code = os.get_line().trim_space()141174 	}175+	// Redirects are refused: a 3xx would hand the one-time code to whatever it points at.142176 	response := http.fetch(143-		url:          '${host}/api/connect'144-		method:       .post145-		header:       http.new_header(177+		url:            '${host}/api/connect'178+		method:         .post179+		header:         http.new_header(146180 			key:   .content_type147181 			value: 'application/x-www-form-urlencoded'148182 		)149-		data:         'code=${urllib.query_escape(code)}'150-		enable_http2: false183+		data:           'code=${urllib.query_escape(code)}'184+		enable_http2:   false185+		allow_redirect: false151186 	) or { return refuse(args.json(), 'could not reach Kepr: ${err.msg()}') }152187 	if response.status_code == 404 {153-		probe := http.fetch(url: connect_url, method: .get, enable_http2: false) or {188+		// One 404 covers a wrong code, an expired one, and a Kepr that hands out nothing. The two189+		// are told apart by asking for the connect page: 302 means the endpoint is there.190+		probe := http.fetch(191+			url:            connect_url192+			method:         .get193+			enable_http2:   false194+			allow_redirect: false195+		) or {154196 			return refuse(args.json(), 'that code is wrong or has expired — nothing was changed')155197 		}156198 		if probe.status_code == 404 {157199 			return refuse(args.json(), 'this Kepr doesn\'t hand out bucket access; use "koh remote s3"')158200 		}201+		if probe.status_code != 302 {202+			return refuse(args.json(), 'this Kepr answered ${probe.status_code} for its connect page — nothing was changed')203+		}159204 		return refuse(args.json(), 'that code is wrong or has expired — nothing was changed')160205 	}161-	if response.status_code < 200 || response.status_code >= 300 {162-		return refuse(args.json(), 'Kepr refused the code — nothing was changed')206+	if response.status_code != 200 {207+		return refuse(args.json(), 'Kepr refused the code (${response.status_code}) — nothing was changed')163208 	}164209 	reply := json2.decode[KeprReply](response.body) or {165210 		return refuse(args.json(), 'Kepr returned an invalid response — nothing was changed')211+	}212+	if reply.endpoint == '' || reply.bucket == '' || reply.region == ''213+		|| reply.access_key_id == '' || reply.secret_access_key == '' {214+		return refuse(args.json(), 'Kepr returned incomplete settings — nothing was changed')166215 	}167216 	settings := machine.RemoteSettings{168217 		kind:     machine.kind_s3

@@ -1271,18 +1271,45 @@12711271 12721272 struct FakeKepr {12731273 	no_access bool1274+	// One reply the redemption must cope with: '500', 'garbage' or 'incomplete'.1275+	fail string1276+	// When set, /api/connect answers 307 here instead of a code reply.1277+	redirect_to string12741278 mut:12751279 	objects map[string][]u81280+	// What the client asked for. A map, because the server holds its own copy of this struct.1281+	seen map[string]string12761282 }12771283 12781284 fn (mut h FakeKepr) handle(req http.Request) http.Response {12791285 	if req.method == .post && req.url == '/api/connect' {1286+		h.seen['code'] = req.data1287+		if h.redirect_to != '' {1288+			return http.Response{1289+				status_code: 3071290+				status_msg:  'Temporary Redirect'1291+				header:      http.new_header(1292+					key:   .location1293+					value: h.redirect_to1294+				)1295+			}1296+		}1297+		if h.fail == '500' {1298+			return http.Response{ status_code: 500, status_msg: 'Internal Server Error' }1299+		}1300+		if h.fail == 'garbage' {1301+			return http.Response{ status_code: 200, status_msg: 'OK', body: 'not json at all' }1302+		}1303+		if h.fail == 'incomplete' {1304+			return http.Response{ status_code: 200, status_msg: 'OK', body: '{"endpoint":"http://x","bucket":"","region":"","access_key_id":"","secret_access_key":""}' }1305+		}12801306 		if req.data == 'code=good' && !h.no_access {12811307 			return http.Response{ status_code: 200, status_msg: 'OK', body: '{"endpoint":"http://${req.header.get_custom('Host', http.HeaderQueryConfig{}) or { '' }}","bucket":"bucket","region":"test","access_key_id":"access","secret_access_key":"secret"}' }12821308 		}12831309 		return http.Response{ status_code: 404, status_msg: 'Not Found', body: '{"error":"no_code","message":"no code"}' }12841310 	}12851311 	if req.method == .get && req.url.starts_with('/connect') {1312+		h.seen['connect'] = req.url12861313 		return http.Response{ status_code: if h.no_access { 404 } else { 302 }, status_msg: 'Found' }12871314 	}12881315 	key := req.url@@ -1319,13 +1346,23 @@13191346 	return http.Response{ status_code: 404, status_msg: 'Not Found' }13201347 }13211348 1322-fn start_fake_kepr(no_access bool) !(int, &http.Server) {1349+fn start_fake_kepr(no_access bool, fail string, redirect_to string) !(int, &http.Server, &FakeKepr) {13231350 	for port in 19401 .. 19411 {13241351 		listener := net.listen_tcp(.ip, '127.0.0.1:${port}') or { continue }1325-		mut server := &http.Server{ listener: listener, handler: &FakeKepr{ no_access: no_access }, show_startup_message: false }1352+		fake := &FakeKepr{1353+			no_access:   no_access1354+			fail:        fail1355+			redirect_to: redirect_to1356+			seen:        map[string]string{}1357+		}1358+		mut server := &http.Server{1359+			listener:             listener1360+			handler:              fake1361+			show_startup_message: false1362+		}13261363 		spawn server.listen_and_serve()13271364 		server.wait_till_running()!1328-		return port, server1365+		return port, server, fake13291366 	}13301367 	return error('no free fake Kepr port')13311368 }@@ -1336,29 +1373,103 @@13361373 	defer { os.rmdir_all(repo) or {} }13371374 	defer { os.rmdir_all(home) or {} }13381375 	init_repo(repo)1339-	port, mut server := start_fake_kepr(false)!1376+	port, mut server, fake := start_fake_kepr(false, '', '')!13401377 	defer { server.stop() }13411378 	host := 'http://127.0.0.1:${port}'1342-	good := run_home(repo, home, 'remote', host, '--code', 'good')1379+	// A trailing slash is the same host, and the saved line keeps the normalised form.1380+	good := run_home(repo, home, 'remote', '${host}/', '--code', 'good')13431381 	assert good.code == 0, good.err13441382 	remote_path := os.join_path(home, '.config', 'koh', 'remote')1383+	credentials_path := os.join_path(home, '.config', 'koh', 'credentials')13451384 	assert os.read_file(remote_path)! == '# written by koh remote — the remote for every repo on this machine\nkind=s3\nendpoint=${host}\nbucket=bucket\nregion=test\nkepr=${host}\n'1346-	assert os.read_file(os.join_path(home, '.config', 'koh', 'credentials'))! == '# written by koh remote — keep this file private\naccess_key=access\nsecret_key=secret\n'1385+	assert os.read_file(credentials_path)! == '# written by koh remote — keep this file private\naccess_key=access\nsecret_key=secret\n'1386+	// The code went as a form field, and the connect page was never asked for: there was a code.1387+	assert (fake.seen['code'] or { '' }) == 'code=good'1388+	assert (fake.seen['connect'] or { '' }) == ''1389+	// A refused code, a missing terminal and a wrong reply all leave both files as they were.13471390 	before := os.read_file(remote_path)!1391+	before_keys := os.read_file(credentials_path)!13481392 	bad := run_home(repo, home, 'remote', host, '--code', 'bad')13491393 	assert bad.code == 113501394 	assert bad.err.contains('wrong or has expired')1395+	// The 404 could also mean a Kepr that hands out nothing, so the connect page is asked for1396+	// with this machine's name. (This machine's name holds no character needing an escape, so1397+	// that part of the query cannot be exercised here.)1398+	// `machine.name()` is `os.hostname() or { 'unknown' }`, so the query carries the same text.1399+	assert (fake.seen['connect'] or { '' }).contains('machine=${os.hostname() or { 'unknown' }}')1400+	no_terminal := run_home(repo, home, 'remote', host)1401+	assert no_terminal.code == 11402+	assert no_terminal.err.contains('needs a terminal')13511403 	assert os.read_file(remote_path)! == before1352-	assert run_home(repo, home, 'remote', host).code == 11404+	assert os.read_file(credentials_path)! == before_keys13531405 }13541406 1407+fn test_remote_kepr_refuses_hosts_that_are_not_hosts() {1408+	repo := temp_repo('kepr-host')1409+	home := temp_repo('kepr-host-home')1410+	defer { os.rmdir_all(repo) or {} }1411+	defer { os.rmdir_all(home) or {} }1412+	init_repo(repo)1413+	for host in ['kepr.uk/foo', 'https://kepr.uk?x=1', 'kepr.uk#frag', '[email protected]', 'kepr uk',1414+		'http://127.0.0.1:1/foo', 'https://'] {1415+		refused := run_home(repo, home, 'remote', host, '--code', 'good')1416+		assert refused.code == 1, host1417+		assert refused.err.contains('is not a Kepr address'), '${host}: ${refused.err}'1418+	}1419+	// --code belongs to the host form alone.1420+	assert run_home(repo, home, 'remote', 's3', 'https://x', 'b', 'r', '--code', 'good').code == 21421+	assert run_home(repo, home, 'remote', 'folder', '/tmp', '--code', 'good').code == 21422+	assert run_home(repo, home, 'remote', '--code', 'good').code == 21423+	assert !os.exists(os.join_path(home, '.config', 'koh', 'remote'))1424+}1425+1426+fn test_remote_kepr_refuses_what_is_not_a_code_reply() {1427+	repo := temp_repo('kepr-reply')1428+	home := temp_repo('kepr-reply-home')1429+	defer { os.rmdir_all(repo) or {} }1430+	defer { os.rmdir_all(home) or {} }1431+	init_repo(repo)1432+	remote_path := os.join_path(home, '.config', 'koh', 'remote')1433+	for mode in ['500', 'garbage', 'incomplete'] {1434+		port, mut server, _ := start_fake_kepr(false, mode, '')!1435+		refused := run_home(repo, home, 'remote', 'http://127.0.0.1:${port}', '--code', 'good')1436+		server.stop()1437+		assert refused.code == 1, mode1438+		assert refused.err.contains('nothing was changed'), '${mode}: ${refused.err}'1439+		assert !os.exists(remote_path)1440+	}1441+	// A redirect would hand the code to wherever it points, so it is refused and never followed.1442+	target_port, mut target, target_fake := start_fake_kepr(false, '', '')!1443+	port, mut server, _ := start_fake_kepr(false, '', 'http://127.0.0.1:${target_port}/api/connect')!1444+	redirected := run_home(repo, home, 'remote', 'http://127.0.0.1:${port}', '--code', 'good')1445+	server.stop()1446+	target.stop()1447+	assert redirected.code == 11448+	assert redirected.err.contains('refused the code')1449+	assert target_fake.seen.len == 0, 'the code was forwarded: ${target_fake.seen}'1450+	assert !os.exists(remote_path)1451+}1452+1453+// An unreachable Kepr is a refusal too, and the settings are still untouched.1454+fn test_remote_kepr_that_cannot_be_reached_changes_nothing() {1455+	repo := temp_repo('kepr-down')1456+	home := temp_repo('kepr-down-home')1457+	defer { os.rmdir_all(repo) or {} }1458+	defer { os.rmdir_all(home) or {} }1459+	init_repo(repo)1460+	refused := run_home(repo, home, 'remote', 'http://127.0.0.1:1', '--code', 'good')1461+	assert refused.code == 11462+	assert refused.err.contains('could not reach Kepr')1463+	assert !os.exists(os.join_path(home, '.config', 'koh', 'remote'))1464+}1465+13551466 fn test_remote_kepr_without_bucket_access_is_explained() {13561467 	repo := temp_repo('kepr-no-access')13571468 	home := temp_repo('kepr-no-access-home')13581469 	defer { os.rmdir_all(repo) or {} }13591470 	defer { os.rmdir_all(home) or {} }13601471 	init_repo(repo)1361-	port, mut server := start_fake_kepr(true)!1472+	port, mut server, _ := start_fake_kepr(true, '', '')!13621473 	defer { server.stop() }13631474 	r := run_home(repo, home, 'remote', 'http://127.0.0.1:${port}', '--code', 'bad')13641475 	assert r.code == 1

@@ -1,6 +1,6 @@11 Module {22 	name: 'koh3'3-	version: '3.0.95'3+	version: '3.0.96'44 	description: 'One SQLite file per repo, one line of history.'55 	license: 'MIT'66 }