xkcd comics data provider

Question

Please help me improving this code(golang), thanks!

Why I need built a API for xkcd

• Can't get the data provided by xkcd directly in front-end(CORS).

• So we can package the data to 10 comics per request. Instead of 1 request match one comic.

main.go

package main

import (
    "encoding/json"
    "fmt"
    "log"
    "net/http"
    "strconv"
    "strings"
)

type comicInfo struct {
    Number int    `json:"num"`
    Img    string `json:"img"`
    Title  string `json:"title"`
    Alt    string `json:"alt"`
}

var comicInfoMap = make(map[int]comicInfo)
var newestComic comicInfo

func fetch(url string, ch chan<- comicInfo) {
    if res, err := http.Get(url); err == nil {
        if res.StatusCode == http.StatusOK {
            newComic := comicInfo{}
            if err := json.NewDecoder(res.Body).Decode(&newComic); err == nil {
                fmt.Println(newComic)
                ch <- newComic
            }
        }
    }
}

func init() {
    ch := make(chan comicInfo)
    if res, err := http.Get("http://xkcd.com/info.0.json"); err == nil {
        if res.StatusCode == http.StatusOK {
            newComic := comicInfo{}
            if err := json.NewDecoder(res.Body).Decode(&newComic); err == nil {
                newestComic = newComic
                comicInfoMap[newestComic.Number] = newestComic
                for j := 0; j < 10; j++ {
                    addtion := 100 * j
                    for i := addtion + 1; i < addtion+101; i++ {
                        indexStr := strconv.Itoa(newComic.Number - i)
                        url := "http://xkcd.com/" + indexStr + "/info.0.json"
                        go fetch(url, ch)
                    }
                    for i := addtion + 1; i < addtion+101; i++ {
                        newComic := <-ch
                        comicInfoMap[newComic.Number] = newComic
                    }
                }
            }
        }
    }
}

func main() {
    http.HandleFunc("/", handler)
    log.Fatal(http.ListenAndServe(":9020", nil))
}

func handler(w http.ResponseWriter, r *http.Request) {
    w.Header().Set("Access-Control-Allow-Origin", "*")
    w.Header().Set("Access-Control-Allow-Methods", "GET, OPTIONS")
    w.Header().Set("Access-Control-Allow-Headers",
        "Origin, X-Requested-With, Content-Type, Accept")

    var encoder = json.NewEncoder(w)
    if r.URL.Path == "/" {
        encoder.Encode(newestComic)
    } else {
        res := strings.Split(r.URL.Path, "/")
        r.Body.Close()
        path, err := strconv.Atoi(res[1])
        if err != nil {
            return
        }
        if path < 100 {
            number := path
            newComicInfoSlice := []comicInfo{}
            for i := 0; i < 10; i++ {
                index := newestComic.Number - i - 10*number
                newComicInfoSlice = append(newComicInfoSlice, comicInfoMap[index])
            }
            encoder.Encode(newComicInfoSlice)
        }
    }
}

Answer 1

Being inspired by recent xkcd comic I will review this particular piece of code:

res := strings.Split(r.URL.Path, "/")
path, err := strconv.Atoi(res[1])

As far as I understand it converts path string "/123" to int 123.

I see several issues here:

• this code creates unnecessary (and possibly large) slice,
• assumes that res[1] is always there,
• parses negative numbers,
• and allows input /123/whatever/goes/here/.

Split

Use strings.SplitN when you know how many parts of the slice you need. On /a/b/c/d/ input strings.Split will create slice with six items, but you need only res[1].

..

url.URL documentation states, that relative paths may omit leading slash. So on .. or . input access to res[1] will trigger panic with "runtime error: index out of range" message.

Malicious HTTP request may contain lots of crap similar ../.. so is crucial to validate this.

I suggest you to check that the path begins with a slash or test the length of res.

stronv.ParseUint

strconv.Atoi allows negative numbers. You can use strconv.ParseUint instead.

---

Path from HTTP request is same thing as any other form of user input. Proper handling of it is full of gotchas.

I suggest you to check that path begins with a slash and use strings.TrimSuffix to remove one possible trailing slash:

// Check that r.URL.Path begins with "/",
if (r.URL.Path[0] == "/") {
    // skip leading slash and trim one(!) possible trailing shash.
    num := strings.TrimSuffix(r.URL.Path[1:], "/")
    // Try to parse non negative number,
    tmp, err := strconv.ParseUint(num, 10, 32)
    if err != nil {
        // and return 404 if there is no one.
    }
    // Convert uint to int.
    index := int(tmp)

    // function body continues...
}