Web server to switch GPIO pin

Question

I am running a simple Python webserver to toggle a GPIO pin on a Raspberry Pi. All seems to work well, but I have a few questions about my code (see below).

I am using the urlparse module to obtain the URL arguments.

The HTML file just sends out Ajax requests using buttons when clicked.

1. I've commented out the send_response lines, and it seems to work both ways. Is it mandatory to send a response?
2. Is the code below easy to exploit? Are there improvements I can make to make it a bit more robust?

Python code:

import time
import SimpleHTTPServer
import SocketServer
from urlparse import *


ledpin=18
myport=8123
# Turn LED on  => http://ip_of_pi:port/control.html?led=on
# Turn LED off => http://ip_of_pi:port/control.html?led=off
class MyRequestHandler(SimpleHTTPServer.SimpleHTTPRequestHandler):
    def do_GET(self):
        urlcomp=urlparse(self.path) # Split URL in components
        query = parse_qs(urlcomp.query) # Get args as dictionary
        if len(query)==0 or query.has_key("led")==False:
            self.path=urlcomp.path
            # self.send_response(200)
        elif query["led"] == ["on"] :
            GPIO.output(ledpin,True)
            # self.send_response(200)
            return
        elif query["led"] == ["off"] :
            GPIO.output(ledpin,False)
            # self.send_response(200)
            return
        SimpleHTTPServer.SimpleHTTPRequestHandler.do_GET(self)

GPIO.setwarnings(False)
GPIO.setmode(GPIO.BOARD)
GPIO.setup(ledpin,GPIO.OUT)

Handler = MyRequestHandler
httpd = SocketServer.TCPServer(('0.0.0.0', myport), Handler)
httpd.serve_forever()

Answer 1

You aren't really using HTTP correctly.

Although your code is not going to allow an attacker to do anything nasty, it does remind me of this story about the Spider of Doom. Observe that there is no authentication required, and that the action is to be requested through an HTTP GET. That means that any web crawler can casually follow a link that switches your LED. Requests to cause a state change on the server should be done using POST, not GET. The convention is that GETs have no side-effects, and that clients may cache responses (rendering the page after making no request) or fetch the URL just to take a look (even if no user actually requested an action).

It is a violation of HTTP for the server to abruptly terminate the connection without sending any response. Doing so makes it impossible for the client to know whether the request succeeded. For all it knows, the requested got dropped by a firewall or some network glitch. If the request succeeded, let the client know through a 200 response code with some success message, or at the very least, a 204 "No content" code.

Answer 2

Import mess

You fail to bring GPIO into the namespace by using something along the lines of from RPi import GPIO. You also import time but don't use it. And you import everything from urlparse using from urlparse import *. This form is frowned upon as it polutes your namespace and may override something else. Better use import urlparse or from urlparse import urlparse, parse_qs.

Global layout mess

It is good practice to put top-level code into functions and call them in a if __name__ == '__main__' clause. That way you can start an interactive session and import your script for testing purposes without having code running as a side effect of your import:

def init_GPIO(ledpin=18):
    GPIO.setwarnings(False)
    GPIO.setmode(GPIO.BOARD)
    GPIO.setup(ledpin,GPIO.OUT)

def init_webserver(port=8123):
    httpd = SocketServer.TCPServer(('0.0.0.0', port), MyRequestHandler)
    httpd.serve_forever()

if __name__ == "__main__":
    init_GPIO()
    init_webserver()

Note that I also turned your constants into parameters with default values. In this case it can make testing much more easier.

You also don't need your alias (Handler = MyRequestHandler) and can use the class directly.

Request handler

There is a more idiomatic way of checking if 'led' is in the query string; by using EAFP:

class MyRequestHandler(SimpleHTTPServer.SimpleHTTPRequestHandler):
    def do_GET(self):
        urlcomp = urlparse(self.path) # split url in components
        query = parse_qs(urlcomp.query) # Get args as dictionary
        try:
            led_state = query['led']
        except KeyError:
            pass
        else:
            turn_on = led_state == ["on"]
            GPIO.output(ledpin, turn_on)
        self.send_response(200)

It is recommended that you call the send_response method since, even if the raspberry-pi handled the request and turned the led on or off, your browser is still waiting for a reply of your webserver. Not calling self.send_response(200) should result in a timeout of your browser at some point.

If you want to send back data to the browser on top of the status code, you can write whatever content you want into self.wfile which will stream is to your browser. You can use it to make it simpler to control your raspi. You can also cusomize the message based on how you handled the action.

class MyRequestHandler(SimpleHTTPServer.SimpleHTTPRequestHandler):
    def do_GET(self):
        urlcomp = urlparse(self.path) # split url in components
        query = parse_qs(urlcomp.query) # Get args as dictionary
        try:
            led_state = query['led']
        except KeyError:
            message = "<p>No commands processed</p>"
        else:
            if led_state in (["on"], ["off"]):
                GPIO.output(ledpin, led_state == ["on"])
                message = "<p>Led turned {}</p>".format(led_state[0])
            else:
                message = "<p>Unknown action {}</p>".format(led_state)
        # Build links whatever the action was
        message += """<p>
                      <a href="/control.html?led=on">Turn on the led</a>
                      </p><p>
                      <a href="/control.html?led=off">Turn off the led</a>
                      </p>"""
        self.send_response(200)
        # Custom headers, if need be
        self.send_header('Content-type', 'text/html')
        self.end_headers()
        # Custom body
        self.wfile.write(message)

If you want to read the body from a file, just use regular python file manipulation:

with open('control.html') as html:
    self.wfile.write(html.read())

Answer 3

Constants in uppercase

ledpin=18
myport=8123

They are never modified, so by convention they should be named uppercase:

LEDPIN = 18
MYPORT = 8123

Spacing

In Python you should write:

x = y

and not:

x=y

Yes, this is minor but it does help readability.

Do not import *

from urlparse import *

If you do this with more than one module, I do not know where the function is coming from. If you are interested in cutting down typing do:

import urlparse as url 

Commented out code

Commented out code is really confusing, like:

# self.send_response(200)

I suggest giving the method an optional argument to see if it should send a response:

def do_GET(self, response=False):
    ...
    if response:
        self.send_response(200)

Docstring

It is nice that you included some references:

# Turn Led on  => http://ip_of_pi:port/control.html?led=on
# Turn Led off => http://ip_of_pi:port/control.html?led=off

But they belong more into a documentation string at the top of your file:

"""
Running a simple python webserver to toggle a gpio pin on a Raspberry Pi.

For further technical info see:
- Turn Led on  => http://ip_of_pi:port/control.html?led=on
- Turn Led off => http://ip_of_pi:port/control.html?led=off
"""
import ...

A docstring is important because it is the first thing shown when a user writes:

import your_module
print(help(your_module))